Projekt do portfolio – code review

0

Hey, mógłbym prosić o code review mojego projektu do portfolio? (backend - Spring + frontend - React)

https://github.com/gjmOfficial/Centrala-schroniska

Chciałbym jeszcze dodać kilka rzeczy do tego projektu, ale już na tym etapie otrzymać jakiś feedback.

Pozdrawiam, gjm

2

Mi się nie podoba koncepcja springowego beana "shelter" połączona z przechowywaniem stanu obiektu w środku. Nie masz synchronizacji pomiędzy operacjami modyfikującymi stan schroniska a pobieraniem raportu. Może to skutkować zwróceniem nieaktualego wyniku(z poprzedniego stanu).

0
p_maciek napisał(a):

Mi się nie podoba koncepcja springowego beana "shelter" połączona z przechowywaniem stanu obiektu w środku. Nie masz synchronizacji pomiędzy operacjami modyfikującymi stan schroniska a pobieraniem raportu. Może to skutkować zwróceniem nieaktualego wyniku(z poprzedniego stanu).

Przy każdej aktualizacji danych schroniska - dodanie/usunięcie zwierzaka następuje zsynchronizowanie danych w schronisku (wewnętrznego stanu beana Shelter)

0

Ref :P

1

Zamiast tego switch'a

https://github.com/gjmOfficial/Centrala-schroniska/blob/master/backend/src/main/java/com/gjm/centralaschroniskabackend/shelter/ShelterController.java

Zrób strategie - politykę

Używasz dziwnych skrótów myślowych.

 bos = new ByteArrayOutputStream() // Bos schroniska. ?

Nie da się tego zrobić prościej w Javie na zasadzie jakiegoś mappera albo obiektu persistencji z adnotacjami?

https://github.com/gjmOfficial/Centrala-schroniska/blob/master/backend/src/main/java/com/gjm/centralaschroniskabackend/shelter/PdfShelterReportConverter.java

Nadużywasz wyjątków lepiej by było dać Either'a.

@Override
    public void registerWorker(Worker worker) {
        if(workerDao.findWorkerByName(worker.getName()) != null) {
            throw new WorkerAlreadyExistsException("Pracownik \"" + worker.getName() + "\" już jest zarejestrowany!");
        }

        workerDao.save(worker);
    }

Zabawne jest to że JPA wymusza na was stosowanie słowa repository tam gdzie jest ono nie potrzebne.

WorkerDao extends JpaRepository<Worker, Long>
0
losowynick napisał(a):

Zamiast tego switch'a

https://github.com/gjmOfficial/Centrala-schroniska/blob/master/backend/src/main/java/com/gjm/centralaschroniskabackend/shelter/ShelterController.java

Zrób strategie - politykę

Używasz dziwnych skrótów myślowych.

 bos = new ByteArrayOutputStream() // Bos schroniska. ?

Nie da się tego zrobić prościej w Javie na zasadzie jakiegoś mappera albo obiektu persistencji z adnotacjami?

https://github.com/gjmOfficial/Centrala-schroniska/blob/master/backend/src/main/java/com/gjm/centralaschroniskabackend/shelter/PdfShelterReportConverter.java

Nadużywasz wyjątków lepiej by było dać Either'a.

@Override
    public void registerWorker(Worker worker) {
        if(workerDao.findWorkerByName(worker.getName()) != null) {
            throw new WorkerAlreadyExistsException("Pracownik \"" + worker.getName() + "\" już jest zarejestrowany!");
        }

        workerDao.save(worker);
    }

Zabawne jest to że JPA wymusza na was stosowanie słowa repository tam gdzie jest ono nie potrzebne.

WorkerDao extends JpaRepository<Worker, Long>

Mógłbym dopytać się o sposób usunięcia switcha?

Jeśli skorzystam z wzorca projektowego - Strategii, to powstanie metoda np. convertShelterReport(), która jako parametr będzie przyjmowała implementację interfejsu strategii np. ShelterReportConverter. Ale znów będę musiał użyć switcha albo jakiejś innej konstrukcji warunkowej, by określić czy przekazać implementację ShelterReportConverter dla PDFa czy dla CSVa... gdzie robię błąd myślowy?

1

Nie znam Javy potraktuj to jako pseudo kod.

class ShelterReportConverter {
    List<RaportPolice> polices;
   
   //ctor
   ShelterReportConverter(List<RaportPolice> polices) {
        this.polices = polices;
   }

   public Report ToReport(string format) {
       return this.polices.filter(x => x != null).map(x => x.Convert(format)).firtst();
}

class PdfPolice implements ReportPolice {

    public Report Convert(string format) {
        if (format == "pdf")
        {
         //logic
         return report;
        }

        return null;
    }
}
0
hiimRealgjm napisał(a):
losowynick napisał(a):

Zamiast tego switch'a

https://github.com/gjmOfficial/Centrala-schroniska/blob/master/backend/src/main/java/com/gjm/centralaschroniskabackend/shelter/ShelterController.java

Zrób strategie - politykę

Używasz dziwnych skrótów myślowych.

 bos = new ByteArrayOutputStream() // Bos schroniska. ?

Nie da się tego zrobić prościej w Javie na zasadzie jakiegoś mappera albo obiektu persistencji z adnotacjami?

https://github.com/gjmOfficial/Centrala-schroniska/blob/master/backend/src/main/java/com/gjm/centralaschroniskabackend/shelter/PdfShelterReportConverter.java

Nadużywasz wyjątków lepiej by było dać Either'a.

@Override
    public void registerWorker(Worker worker) {
        if(workerDao.findWorkerByName(worker.getName()) != null) {
            throw new WorkerAlreadyExistsException("Pracownik \"" + worker.getName() + "\" już jest zarejestrowany!");
        }

        workerDao.save(worker);
    }

Zabawne jest to że JPA wymusza na was stosowanie słowa repository tam gdzie jest ono nie potrzebne.

WorkerDao extends JpaRepository<Worker, Long>

Mógłbym dopytać się o sposób usunięcia switcha?

Jeśli skorzystam z wzorca projektowego - Strategii, to powstanie metoda np. convertShelterReport(), która jako parametr będzie przyjmowała implementację interfejsu strategii np. ShelterReportConverter. Ale znów będę musiał użyć switcha albo jakiejś innej konstrukcji warunkowej, by określić czy przekazać implementację ShelterReportConverter dla PDFa czy dla CSVa... gdzie robię błąd myślowy?

Tak, w jakimś miejscu taka logika musi być, możesz użyć do tego celu wzorca fabryki w ktorej zawrzesz logikę jaką implementację zwrócić.

1
hiimRealgjm napisał(a):
p_maciek napisał(a):

Mi się nie podoba koncepcja springowego beana "shelter" połączona z przechowywaniem stanu obiektu w środku. Nie masz synchronizacji pomiędzy operacjami modyfikującymi stan schroniska a pobieraniem raportu. Może to skutkować zwróceniem nieaktualego wyniku(z poprzedniego stanu).

Przy każdej aktualizacji danych schroniska - dodanie/usunięcie zwierzaka następuje zsynchronizowanie danych w schronisku (wewnętrznego stanu beana Shelter)

Chodziło mi o taki przypadek w którym ktoś się wstrzeli między opracją modyfikującą a aktualzująca stan i dostanie niepoprawny wynik ponieważ kontener nie będzie jeszcze odświeżony.

0

Dlaczego commity są pisane po polsku ? Readme też nie powinno być po polsku.

0

Bardzo nietrafnie uważasz. Prawdę mówiąc jako potencjalny pracodawca przeglądający Twój kod zanim jeszcze tam zajrzę od razu podejrzewam że coś jest nie tak z taką osobą. Kod pisze się po angielsku, i tak samo nazywa się commity i w tym języku pisze się readme. Co jeśli Twoja aplikacja będzie przekazana innemu zespołowi używającyemu innego języka ?

1

To już inna kwestia. Nie - nie powinny komunikaty/elementy/ui itd powinny polegać na kodach/id które są odniesieniem do plików zasobów które można łatwo przetłumaczyć.

0
public long jwtToId(String jwt) {
        try {
            if(jwt == null) {
                throw new NotAuthenticatedException("Błąd bezpieczeństwa JWT!");
            }

            String stringifedId = Jwts.parser()
                    .setSigningKey(key)
                    .parseClaimsJws(jwt)
                    .getBody()
                    .getSubject();

            return Long.parseLong(stringifedId);
        } catch(JwtException exc) {
            throw new NotAuthenticatedException("Błąd bezpieczeństwa JWT!");
        }
    }

Proponuję dać na mszę żeby nigdy nie poleciał JwtException. Jeśli poleci to życzę udanych kilku(nastu) godzin debugowania. :-)

0
W2K napisał(a):
public long jwtToId(String jwt) {
        try {
            if(jwt == null) {
                throw new NotAuthenticatedException("Błąd bezpieczeństwa JWT!");
            }

            String stringifedId = Jwts.parser()
                    .setSigningKey(key)
                    .parseClaimsJws(jwt)
                    .getBody()
                    .getSubject();

            return Long.parseLong(stringifedId);
        } catch(JwtException exc) {
            throw new NotAuthenticatedException("Błąd bezpieczeństwa JWT!");
        }
    }

Proponuję dać na mszę żeby nigdy nie poleciał JwtException. Jeśli poleci to życzę udanych kilku(nastu) godzin debugowania. :-)

Mógłbyś rozwinąć?

0

Zastępujesz wyjątek który mówi cokolwiek i ma jakiekowliek sensowne dane, wyjątkiem który nie mówi absolutnie nic. Po co łapiesz go w tym miejscu ?

0

@W2K: A właściwie dlaczego to nazwy commitów nie po polsku? Ja w pracy piszę po ang, bo tak jest ustalone, ale u siebie w domowych projektach często piszę polskie, czasem skrótowo, bo robię to dla siebie. Nie bądź niewolnikiem narzędzi ;)

0

By ControllerAdvice go złapał (out-of-the-box) i mi zwrócił odpowiedni status na frontend (do Reacta).

1
danek napisał(a):

@W2K: A właściwie dlaczego to nazwy commitów nie po polsku? Ja w pracy piszę po ang, bo tak jest ustalone, ale u siebie w domowych projektach często piszę polskie, czasem skrótowo, bo robię to dla siebie. Nie bądź niewolnikiem narzędzi ;)

Projekt do szufladki pisz jak chcesz. Projekt do portfolio ma pokazać że znasz podstawy i dobre praktyki i pokazać się pracodawcy od najlepszej strony.

4

Zajmę się tylko częścią backend'ą. Będę wyjątkowo złośliwy skoro idzie to do portfolio

  1. Nazwa projektu też powinna być po angielsku
  2. README.md Pierwsza zasada krętacza Okładka ma być piękna. Jeśli okładka jest dopracowana, to ludzie myślą, że reszta też. I odwrotnie. W twoim przypadku okładka to README.md
    2.1. logining typo
    2.2. W Features piszesz wszystko równoważnikiem zdania, bo czym nagle Automatically sent email... jest zdaniem. Zamień na Automatically sending email...
    2.3. Co to znaczy Images of animals, Profile photos of workers? Również użyj równoważnika zdania
    2.4. Piszesz od mobile-app, to napisz też o frontendzie webowym
    2.5. Nie pisz, że Spring/Hibernate to Java. Każdy o tym wie. Tak samo że React to JS
    2.6. Skoro to portfolio juniorskie, dopisałbym w Tech Stacku o mavenach, bazie danych, yarnach, lombokach, herokach i co tam masz jeszcze
    2.7. Usuń Unit tests z TODO. To strasznie razi i pojawia się lampka: Ocho! On nie pisze testów na bieżąco!
    2.8. Developer's info: dodaj informacje, która to jest klasa główna
    2.9. Używaj tego śmiesznego znacza "`", żeby otoczyć wszystkie "kodowe" inlinowe rzeczy, oraz "```" na code snippety
    2.10. Podziel Deployment tak, żeby tylko kod był w bloku kodu, a twoje komentarze i wyjaśnienia normalnym tekstem
  3. com.gjm.centralaschroniskabackend nie używaj polskiej nazwy
    3.1. CentralaSchroniskaBackendApplication nie używaj polskiej nazwy
    3.2. Wszystkie dependencje do beanów jakie masz powinny być final
    3.3. Jak używasz lomboka, to używaj lomboka: Beany mogą mieć @RequiredArgsConstructor. Nie musi być @Autowired, żeby spring sie ogarnął
  4. com.gjm.centralaschroniskabackend.shelter
    4.1. Shelter nazwa jest beznadziejna. Sugeruje, że będzie to POJO, które opisuje np. ile i jakich boxów jest dla zwierząt. Proponuję SchelterService
    4.1.1. MAX_ANIMALS powinno być camelCase, bo nie jest stałą (w sensie static final)
    4.1.2. Serwisy powinny być bezstanowe. Usuń pola animals oraz schelterStatus. Nie ma potrzeby tworzenia takiego śmieszego cache, a jeśli jest to powinno zostać zrobione pomiędzy serwisem a warstwą dostępu do danych. De-facto ich nie potrzebujesz, obliczaj wartości dopiero wtedy gdy są potrzebne (czyli przy addAnimal i getShelterRaport). Wtedy usuniesz też tego paskudnego @PostConstruct
    4.1.3. Schelter łamie SRP. Powinieneś rozdzielić odpowiedzialności i stworzyć AnimalService który by dodał i usuwał Animal, ReportService, który generowałby raport na życzenie. Wtedy nie zostaje nic dla Schelter i można go usunąć. AnimalService i RaportService powinny mieć swoje Controllery
  5. com.gjm.centralaschroniskabackend.shelter.report powinno być równoległe do paczki schelter
    5.1. Konwertery nie powinny być świadome tego w jaki sposób ich produkt będzie procesowany dalej. Nie wiedzieć nic o HttpHeaderach. Zrób osobne HttpHeaderFactory czy coś, które ci będzie to produkowało. W ten sposób unikniesz patologii jakiej jest return null w WebShelterReportConverter. Nawiasem mówiąc return null to rak i skandal i powinno się za to linczować =P Nawiasem mówiąc setujesz nulla w ResponseEntitity#headers gdy format ci przyjdzie web.
    5.2. Używaj pętli for (Object : objects) zamiast tej z iteratorem. A najlepiej lambdy w .forEach
    5.3. Metoda convert jest strasznie długa. Wydziel poszczególne fragmenty generowania raportu do osobnych metod (np osobno headery, wartosci, uwagi)
    5.4. Masz bardzo dużo powtórzeń. Trzeba je zmieniać w każdym raporcie z osobna. Zamiast tego wydziel do czegoś ogólnego liste headerów ("Lp.", "Imie", "Plec", "Wiek", "Data przybycia"), logike konwerującą Gender do tekstu (animal.getGender().equals(Gender.FEMALE) ? "Zenska" : "Meska"), konwerter czasu, uwagi, itp
  6. com.gjm.centralaschroniskabackend.security
    6.1. Dependency injection się kłania. key powinno przychodzić w parametrze. Zrób metodę fabrykującą Beana, inaczej tego sprawnie nie przetestujesz
  7. com.gjm.centralaschroniskabackend.aspects
    7.1. Każdy moduł powinien mieć swoje @ControllerAdvice, a nie jeden wielki GodAdvice, który ma potencjał przekroczyć 1k lini kodu
    7.2. 500 sie zwraca jak cos sie mocno pochrzanilo na serwerze. Ty masz rozpatrywane normalne przypadki
    7.3. shelterIsFullExceptionHandler, workerAlreadyExistsExceptionHandler -> 409
    7.4. workerDoesntExistExceptionHandler, animalDoesntExistExceptionHandler -> 404
    7.5. wrongWorkerPasswordExceptionHandler -> 401
  8. com.gjm.centralaschroniskabackend.animal
    8.1. Animal pole image powinno zawierać w sobie, że jest to base64, żeby poprawnie je setować i wiedzieć co się wyciąga. Komentarz usunąć
    8.2. AnimalController nie powinien być zależny od AnimalService a nie od Schelter, pisalem to juz wyzej
    8.3. Nie potrzebujesz zwracać ResponseEntity. Wystarczy void. Jak potrzebujesz zmienić tylko StatusCode to masz adnotacje @ResponseStatus
    8.4. Restowa końcówka powinna być animals, a nie animal
  9. com.gjm.centralaschroniskabackend.worker
    9.1. LoginDto masz tam publiczne pola =P
    9.2. Worker - To nie jest obiekt opisujący pracownika, tylko konto użytkownika. Worker bardziej pasuje do obiektu który przechowuje dane o pracowniku, a nie do obiektu który służy do autentykacji. Proponuję nazwę Account
    9.3. Worker to samo co animal i image
    9.4. Zrób wspólny interfejs dla animal i worker: coś a la Imageable i coś a la dateOfAddable
    9.5. WorkerController restowa końcówka workers
    9.6. dodanie użytkownika: @PostMapping("/workers")
    9.7. logowanie: @PostMapping("/login") gdzie w module odwiadajacym za uwierzytelnianie
    9.8. nawiasem mówiąc rozdziel zarządzanie użytkownikami od kwestii unwierzytelniania i autoryzacji. Weź pod uwagę, że taki moduł animal może w kontrollerze dodać użytkownika. Chyba nie bardzo dobrze =(
    9.9. Zwracaj obiekt zamiast 1ResponseEntity1 jak nie porzebujesz setowac headerow
    9.10. WorkerEmailNotificationsDemon używaj lamda, zamiast forów. lamdy wyglądają sexy i jakbyś wiedział co robisz =D
    9.11. Rzutowanie na byte[]... Masakra. Zamiast tego użyj generyków w ReportConverter
    9.12. Hey! Informujemy ciebie... nie mieszaj języków
    9.13. Scheduler jest bez sensu, bo możesz wysyłać notyfikację przy dodawaniu zwierzyny. W przypadku przepelnieniu schroniska nie bedziesz dostawac irytujacego mejla co 5 godzin, tylko raz
    9.14. Najpierw generujesz raport, a potem sprawdzasz czy w ogole go wyślesz
    9.15. throw new RuntimeException(exc); to rak, rzuć coś własnego jak nie chcesz checked exception
    9.16. WorkerServiceDbImpl wydziel osobny serwis do autentykacji, a drugi serwis do CRUDA użytkownika
    9.17. hasuj hasło na bazie danych i porównuj hashe. Mało roboty, a duże zyski (w oczach rekrutera w porównianiu z przechowywaniem plain-textu). Jak jeszcze posolisz, użyjesz bscrypta to juz w ogóle
    9.18. Walidacje name zrób na constraincie na bazie danych
    9.19 WorkerDao findWorkerByName powinno zwracać Optionala
  10. BRAKUJE CI WALIDACJI! Można śmiało powrzucać np. nulle jako e-mail i wtedy ci sypnie NPE, albo e-mail który nie jest e-mailem, i wtedy "coś" też się wywali. Użyj javax.validation
  11. BRAKUJE CI TESTÓW! To jest chyba największa wada całego projektu. Nieważne, że projekt działa i był testowany manualnie. Bardzo dużo osób będzie to po prostu strasznie razić.
  12. Proponuję ci używać vavra. Vavr jest sexy, nawet bardziej niż lambdy. Dobrze będzie to wyglądało na portfolio
0

Dobra na dziś 7 pierwszych punktów wykonanych...reszta jutro ;p

0

"Każdy moduł powinien mieć swoje @ControllerAdvice, a nie jeden wielki GodAdvice, który ma potencjał przekroczyć 1k lini kodu" - można zrobić jeden ControllerAdvice, który będzie obsługiwał np. wyjątki dziedziczące po GenericShelterException? Każdy wyjątek mojej aplikacji dziedziczyłby wtedy po tym GenericShelterException, a dodatkowo przechowywałbym tam kod HTTP, który w konstruktorze byłby nadawany?

Gdy próbowałem zrobić jak mówisz - zwracać void/obiekt z metod restowego kontrolera to wszystko jest OK, z wyjątkiem zwracania Stringa (wiadomość z obiektu wyjątku) z metody controller advice'a.... wtedy Spring zwracał mi cały obiekt różnych informacji, zamiast po prostu serializować wiadomość do response'a... :(

WYDAJE MI SIĘ, że Spring może traktować zwracany String jako nazwa widoku, jednak nie jestem pewien, ze względu na to, że nie mam tutaj skonfigurowanego ViewResolvera, a żadnego silnika szablonów typu Thymeleaf ręcznie nie dołączałem (by mieć defaultową konfigurację)...

Jak na razie rozwiązałem ten problem tym "GenericShelterException" i podoba mi się to rozwiązanie, jednak chciałbym się dowiedzieć, dlaczego Spring robi to co robi przy zwracaniu Stringa z metody :)

Z góry dzięki :)

1

Użyj @RestControllerAdvice zamiast @ControllerAdvice. Bajka ta sama, co w przypadku @RestController i @Controller.

PS: GenericShelterBrainException to brzydka nazwa. Samo SchelterBrainException opisuje ci wyjątek, który jest właściwy dla modułu SchelterBrain. W twoim przypadku moduł SchelterBrain to cała aplikacja. Nie ma więc sensu dodawać słowa Generic, bo samo SchelterBrain o tym mówi

0

"Zrób wspólny interfejs dla animal i worker: coś a la Imageable i coś a la dateOfAddable" - totalnie nie czaję tego punktu :((

1

Jeśli stworzysz takie interfejsy:

public interface Auditable {
    LocalDateTime getCreationDate();
    LocalDateTime setCreationDate(LocalDateTime creationDate);
}

public interface Imageable { //brzydka nazwa, ale nie umiem w tej chwili wymyślić nic lepszego
    String getBase64ImageString();
    String setBase64ImageString(String base64ImageString);
}

i zaimplementujesz je w Animal i Worker, to wymusisz wspólny kontrakt na oba te zachowania, co może być przydatne jeśli chcesz np.:

  1. Pchać te obiekty na front, gdzie tam jakiś wspólny komponent ogarnia date utworzenia, albo obrazek i mieć pewność, że nazwa propertisa jest zawsze taka sama
  2. Możesz stworzyć jeden ogólny serwis, który ogarnia dodawanie/zmienianie obrazka danemu czemuś (zamiast implementować to osobno dla Worker i Animal masz zaimplementowane raz, a rozszerzenie tego na następne obiekty wymaga jedynie zaimplementowania interfejsu Imageable
0

Okkk, większość już zrobiłem. Od jutra zabieram się za Vavr i testy jednostkowe :P

1
  1. Nie udostępniaj publicznie w repo credentiali do bazy, szczególnie takiej która stoi na AWS ;) Analogicznie z mailem :)

https://github.com/gjmOfficial/Shelter-brain/blob/master/backend/src/main/java/com/gjm/shelterbrainbackend/report/converter/factory/ShelterReportConverterFactory.java
https://github.com/gjmOfficial/Shelter-brain/blob/master/backend/src/main/java/com/gjm/shelterbrainbackend/report/converter/factory/ShelterReportHttpHeadersFactory.java

To mi się nie podoba. Raz ze enum > string zawsze, a dwa że masz tu identyczne switche/ify a pytanie czy w ogóle powinieneś je mieć? Nie da się tego strategią załatwić? Przecież taki konwerter może wiedzieć jaki content type produkuje?

1 użytkowników online, w tym zalogowanych: 0, gości: 1