Wątek przeniesiony 2023-01-22 22:00 z Java przez cerrato.

Aplikacja do zarządzania wydarzeniami w branży IT - prośba o code review

0

Robię sobie taką prostą aplikację do zarządzania wydarzeniami w branży IT. Mam jeszcze w planach dorobić możliwość wysyłania wiadomości między użytkownikami, ale to zostawiam sobie na koniec. W pierwszej kolejności przerobię ją na REST API, bo chcę mieć API w portfolio i próbować wysyłać gdzieś CV. Też słyszałem, że rekruterzy nie patrzą jakoś bardzo na portfolio, więc tym bardziej nie chcę siedzieć wieczność nad jednym projektem. Czy moglibyście zrobić code review tego co zrobiłem do tej pory?

Link do repozytorium https://github.com/Dreilt/basic-spring-mvc-app

2

Jak przerobisz ... to na tamto, tamto na owo ... to wtedy się zgłaszaj.

0
_13th_Dragon napisał(a):

Jak przerobisz ... to na tamto, tamto na owo ... to wtedy się zgłaszaj.

Różnice będą głównie w kontrolerach, a całej reszty nie można teraz ocenić?

5

Tak z grubsza co mi się rzuciło w oczy:

  1. Brak Readme
  2. Po co używasz dwóch różnych baz danych H2 i Mysql, lepiej użyć tylko jednego z nich
  3. Masz dodanego lomboka, ale używasz go tylko w 2 klasach, a możesz używać również w innych przypadkach - jak dla mnie albo używaj go albo wywal z projektu
  4. W application.yaml masz konfigurację, która jest nadpisywana w innych profilach, trzymaj w niej tylko to co jest wspólne dla każdego profilu
  5. Klasa BaseEntity - wtf?? :)
  6. Zbudowałeś projekty przed pushem? jest wskazana nie istniejąca metoda w testach
  7. Wydaje mi się, że zrobienie AppUserRole jako enum byłoby lepszym rozwiązaniem
  8. Jak masz dużo argumentów np w konstruktorze to niech będą w oddzielnych liniach
  9. Wszystkie klasy są publiczne, można porobić je package-private
  10. Masz pakcage exception, a w nim jeden exception, a resztach porozrzucana po projekcie - jak dla mnie dziwne, ale to może tylko ja :)
  11. CustomHandlerExceptionResolver, if-ów można się pozbyć stosując wzorzec strategię, no i czy na pewno chcesz zwracać nulla?
  12. Jak masz Optional to nie zawsze korzystasz z orElseThrow
  13. Jak klasy BasicSpringMvcAppApplicationTests nie używasz to usuń

EDIT:
I na przyszłość, tak jak wyżej napisał @_13th_Dragon nie dawaj do review czegoś co nie jest skończone

0

@Kamil Czubaszek:

  1. Dodam w przyszłości.
  2. Baza H2 jest dla profilu dev, a MySQL dla profilu prod.
  3. Staram się jak najmniej korzystać z Lomboka, bo podobno są z nim duże problemy.
  4. Skończyła mi się licencja na IntelliJ IDEA Ultimate, a wersja Community nie działa dobrze z profilami i przypadkiem nadpisałem.
  5. https://www.sztukakodu.pl/equals-hashcode/
  6. Nie budowałem, ale zbuduję i sprawdzę to.
  7. Zastanawiam się gdzie najlepiej byłoby umieścić ten wyjątek i chyba najlepiej w pakiecie appuser.
  8. Właśnie ten null mi się tam nie podoba, ale też nie bardzo wiem jak się go pozbyć. Doradzisz coś?

Można powiedzieć, że ta aplikacja jest skończona. Wiadomości to pierwsza rzecz jaka mi przychodzi do głowy, którą chciałbym zaimplementować, ale najpierw chcę poprawić ten kod i zrobić go jak najlepszym. REST API to osobny temat i jak je zrobię to pewnie też poproszę o code review.

Chciałbym jeszcze żebyś zerknął do EventService, np. do metody findOrganizerEvent(). Najpierw pobierane jest wydarzenie, a później jest sprawdzenie czy użytkownik jest organizatorem. Pytanie - czy można to zrobić lepiej/ładniej? Jest kilka metod, w których powtarza się ten schemat.

Zrobiłem sobie taką fasadę AuthenticatedUserFacade, która zwraca użytkownika z SecurityContextu. Zastanawiam się czy tam też nie można czegoś poprawić.

0
Dreilt napisał(a):

@Kamil Czubaszek:

  1. Dodam w przyszłości.
  2. Baza H2 jest dla profilu dev, a MySQL dla profilu prod.
  3. Staram się jak najmniej korzystać z Lomboka, bo podobno są z nim duże problemy.
  4. Skończyła mi się licencja na IntelliJ IDEA Ultimate, a wersja Community nie działa dobrze z profilami i przypadkiem nadpisałem.
  5. https://www.sztukakodu.pl/equals-hashcode/
  6. Nie budowałem, ale zbuduję i sprawdzę to.
  7. Zastanawiam się gdzie najlepiej byłoby umieścić ten wyjątek i chyba najlepiej w pakiecie appuser.
  8. Właśnie ten null mi się tam nie podoba, ale też nie bardzo wiem jak się go pozbyć. Doradzisz coś?

Można powiedzieć, że ta aplikacja jest skończona. Wiadomości to pierwsza rzecz jaka mi przychodzi do głowy, którą chciałbym zaimplementować, ale najpierw chcę poprawić ten kod i zrobić go jak najlepszym. REST API to osobny temat i jak je zrobię to pewnie też poproszę o code review.

Chciałbym jeszcze żebyś zerknął do EventService, np. do metody findOrganizerEvent(). Najpierw pobierane jest wydarzenie, a później jest sprawdzenie czy użytkownik jest organizatorem. Pytanie - czy można to zrobić lepiej/ładniej? Jest kilka metod, w których powtarza się ten schemat.

Zrobiłem sobie taką fasadę AuthenticatedUserFacade, która zwraca użytkownika z SecurityContextu. Zastanawiam się czy tam też nie można czegoś poprawić.

​ad 2 - raczej używa się takiej samy bazy dla każdego profilu, żeby uniknąć potencjalnych problemów
ad 3 - mogą wystąpić problemy, ale jak nie planujesz zmieniać wersji javy czy spring boot to problemy nie powinny wystąpić - na 100% nigdy nie wiadomo
ad 4 - kiepska wymówka :) pracuje na Community i nie miałem problemu z profilami
ad 5 - nie natrafiłem jeszcze na coś takiego, stąd moje wtf, ale już rozumiem dlaczego tak zrobione
ad 6 - lepiej
ad 7 - jakiś defaultowy Exception handler, żeby błąd nie umknął

Jeśli chodzi o EventService to robisz po prostu mniejsze prywatne metody jeśli tylko w tej klasie są potrzebne i je używasz gdzie potrzeba.
Jak sprawdzasz czy Optional jest empty to lepiej zrobić orElseThrow

0

0 testów?

pała, siadaj.

PS: A nie, jednak są. To 4+.
PS: A nie, jednak te testy to jest mock-hell. Takze 2- jednak będzie.

String email = currentUser.getName();
Optional<AppUser> userOpt = appUserRepository.findByEmail(email);
if (userOpt.isEmpty()) {
    throw new AppUserNotFoundException(String.format("User with email %s not found", email));
}

return userOpt.get();

możesz skrócić do

return appUserRepository
  .findByEmail(currentUser.getName())
  .orElseThrow(new AppUserNotFoundException(String.format("User with email %s not found", email)));
0

@Kamil Czubaszek:

ad 4. uruchamiasz aplikację komendą?
ad 11. poprawiłem https://github.com/Dreilt/basic-spring-mvc-app/commit/a2a39932e84f6719b1a302e374d7fd99c4dc8c99 teraz jest dobrze, czy jeszcze można coś poprawić?

@Riddle: co to znaczy mock-hell? Że brak testów integracyjnych? Mógłbyś rozwinąć?

1
Dreilt napisał(a):

@Riddle: co to znaczy mock-hell? Że brak testów integracyjnych? Mógłbyś rozwinąć?

To znaczy że Twoje testy praktycznie nic nie testują; że jest to znaczny przerost formy nad treścią; że 90% tego co tam jest to są setupy mocków, a może 10% to jest faktyczne testowanie czegokolwiek.

Np zobacz sobie na ten test:

@Test
void shouldReturnSavedDefaultProfileImage() {
    // given
    when(profileImageRepository.save(any(ProfileImage.class))).thenAnswer(i -> i.getArguments()[0]);
    // when
    ProfileImage returnedCreatedDefaultProfileImage = profileImageService.createDefaultProfileImage();
    // then
    verify(profileImageRepository, times(1)).save(argThat((ProfileImage savedProfileImage) -> {
        Assertions.assertAll("Testing saved default profile image",
                () -> assertNull(savedProfileImage.getId()),
                () -> assertEquals(returnedCreatedDefaultProfileImage.getFileName(), savedProfileImage.getFileName()),
                () -> assertEquals(returnedCreatedDefaultProfileImage.getFileType(), savedProfileImage.getFileType()),
                () -> assertEquals(returnedCreatedDefaultProfileImage.getFileData(), savedProfileImage.getFileData())
        );
        return true;
    }));
}

Jak dla mnie to ten test testuje praktycznie nic, i możnaby go wywalić bez absolutnie żadnej straty moim zdaniem.

Powinieneś oczywiście napisać test, sprawdzający czy da się stworzyć profil z domyślnym obrazkiem, ale na pewno nie w taki sposób. Już pomijam całkowicie, że ten test za dużo wie o implementacji (dużo za dużo), przez co refaktor kodu jest znacznie utrudniony. Potem będziesz chciał coś zmienić, i zobaczysz że Twoja zmiana powoduje fail połowy testów, i będziesz miał wybór - albo fixować testy, albo nie zrobić refaktoru - oba wyjścia są słabe, bo testy są słabe.

0

@Riddle: to lepiej byłoby zrobić testy integracyjne dla serwisów i kontrolerów?

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