Prośba o code review małej aplikacji

1. Po co te buildery do struktur z jednym/dwoma polami
2. To Thread.sleep() w teście zalatuje brzydkim zapachem
3. Dokładasz @slf4j a nic nie logujesz
4. Jak już chcesz tego lomboka to przykładowo na takiej klasie Post wystarczyloby @Value zamiast reszty @ i jeszcze private byś mógł wywalić
5. Takie chainy stream().sorted().reversed().collect() lepiej sie czyta z góry na dół niż od lewej do prawej. Czyli nowa linia przed każdą kropką

Nie wczytywałem się za bardzo. To tylko kilka rzeczy, które rzuciły mi się w oczy. Ogólnie wygląda ok :)

Dzięki. Poprawiłem.
Threed.sleep musiał zostać, nie znalazłem lepszego sposobu na wymuszenie kolejności postów(czasami dwa sąsiednie posty miały ten sam timestamp)

Zamiast używać LocalDateTime.now() w logice możesz sobie stworzyć jakiś TimeProvider, wstrzykiwać go i pobierać czas z niego. Wtedy w teście można zamockować jakiekolwiek timestampy

1. Mam nadzieje, że to nie jest wszystko, bo aplikacja jest baaaardzo mała
2. AddPostRequest to nie jest obiekt żądania, tylko jego body: zmień nazwę albo na AddPostRequestBody albo najprościej na AddPostDto. Mógłbyś też po prostu wrzucić zwyklego stringa w body
3. Po co @JsonIgnoreProperties(ignoreUnknown = true) @Builder w AddPostRequest
4. sygnatura metody to followUser a w urlu tracks. Brak konsekwencji
5. Nie dziel poziomo (czyli paczka z repami, serwisami, modelami, controllerami), bo jak skończysz z 30 repozytoriami, to sie zgubisz. I nie masz mozliwości blokowania dostępu, tylko pionowo. Również podziel swoje god-objecty per ficzer (osobny controller od obslugi postow, osobny od userow)

• user
  -- User
  -- UserRepo
  -- UserService
  -- UserController
• post
  -- Post
  -- PostRepo
  -- PostService
  -- PostController

1. SocialNetworkAppApplication?? AplikacjaApkaSiećSpołeczna? Po co powtarzasz nazwę?
2. PostsRepository -> PostRepository. Tylko to nie jest repozytorium postów. To jest repozytorium wszechrzeczy.
3. Co niby ma zwracać followUser, co to za zbiór stringów? Nie ma to sensu.
4. getPostsByUserName przyjmuje w parametrze id zamiast name
5. ExceptionHandlerController Błąd walidacji to zazwyczaj 422, a nie 400. 400 jest wtedy, gdy nie jesteś w stanie wejść poprawnie do kontrollera
6. Po co POST "/users/{userId}/posts" zwraca jakąś liste Postów? Zwróć voida (albo id) oraz 201
7. "/users/{userId}/tracks/{userIdToFollow}" zwróć 201
8. "Chopuj" buildery
9. Jak nie ma użytkownika, to poponuje rzucic exception "EntityNotFound" czy coś, a nie walidation exception. I wtedy zwracać RESTem 404
10. Obsługę foreign-key powinieneś zaimplementować w repozytorium (czyli sprawdzanie userId)
11. Po co metoda getSocialNetworkValidationExceptionSupplier zwraca suppliera? Niech zwraca exception i wtedy zrobisz () -> getSocialNetworkValidationExceptionSupplier(userId) w orElseThrow
12. Byłoby fajnie jakby error code był enumem, czy coś. Żeby to faktycznie był code
13. Swoje mapy w repozytorium wszechrzeczy ponazywaj tak, zeby bylo wiadomo co jest kluczem. Np usersByUserId
14. Możesz użyć "dwu-dwukropkowej" lambdy w getFollowedPostsByUserName
15. zamiast isUserPresent proponuje użyć słowa klucza exists, np: existsById
16. Jedyne co robi getFollowedUsersByUserId to wywołuje retrieveFollowedUsersByUserId. Obi są prywatne. Po co?
17. Używaj modyfikatorów dostępu. Domyślnie defaultowy (czyli żaden). Jak już musisz udostępnić coś poza paczkę, to tylko serwis (a najlepiej jego interfejs), potrzebne POJOsy i wyjątki. Nie widze powodu, dla ktorego kontrollery moglyby byc publiczne
18. Powiel testy na warstwe. Testy controllera/serwisu/repo. Zrób je unitami, czyli nie stawiaj calego wielkiego kontekstu springa, bo kiedys czegos zapomnisz zamockować. Do testów controllera masz springowe "mockMvc"
19. Jak już będziesz miał unity, to możesz zrobić testy integracyjne za pomocą @SpringBootTest i TestRestTemplate. Ale w sumie nie musisz
20. Ale nie rób ich dziedzicząc po magicznej klasie. Generalnie unikaj dziedziczenia i rozwiąż to kompozycją
21. No i twoja klasa abstrakcyjna w sumie nie ma abstrakcji. To tylko kolejny god-object z wszystkimi możliwymi żądaniami jakie istnieją. Po co SocialNetworkAppApplicationAddPostsTests ma mieć dostęp do żądania callFollowUser??
22. Przydałby się jeden test integracyjny, który sprawdza, czy apka wstaje. Samo @SpringBootTest i jeden pusty test

TLDR:
Najpierw popraw architekturę, podziel poziomo (per ficzer). Reszta wyjdzie z tego sama

Dzięki,
... co do tej architektury per feature mam wątpliwości, nie wiem czy to nie będzie przerost formy na trescia, bo w sumie nie mam żadnej funkcjonalności związanej z użytkownikami wystawionej na zewnątrz, jest to tylko kontener w wartwie repo, aby sprawdzic czy dany user istnieje czy nie. Userzy są dodawani automatycznie przy pierwszym poscie. Jak sądzisz ?

Myśle, że przy tak małej aplikacji nie ma to znaczenia. Przy większych takie paczki mają już sens. Potem dużo łatwiej z takiej paczki zrobić osobny moduł(np mavenowy), a moduł można wyjąć jako osobny (micro)serwis :) Przy pakowaniu wszystkich repo do paczki repo, wszystkich kontrolerów do paczki kontrolerów wydzielenie jakiegoś modułu/serwisu to by była mordęga

Poprawiłem większość punktów, łacznie z podziałem pakietów na per feature. Myślę, że wygląda to teraz dużo lepiej.
Dzięki wszystkim za uwagi.

Jedziemy dalej:

1. w UserController brakuje ci <> przy ResponseEntity. Tego typu błędy wyłapuje IntelliJ, nie rozumiem czemu w ogóle to masz
2. A tak w sumie, to nie potrzebujesz ResponseEntity, tylko możesz zwrócić User i zaadnotować to @ResponseStatus(201). To samo w PostController
3. Czemu pola User nie są prywatne? Czemu obiekt nie jest immutable?
4. Twój UserEntity tylko udaje immutable, bo ma w polu mutable Set
5. Mieszasz konfigurację za pomocą klas @Configuration i adnotacji nad klasami. Zdecyduj się na jedno rozwiązanie, żeby było konsekwentnie
6. Po co metoda mapUserEntityToUser zwraca funkcje, zamiast po prostu przyjąć UserEntity w parametrze? KISS
7. Chopuj buildery
8. W InMemoryUsersRepository#trackUser user i userEntity to to samo =P. Zrobisz dwa razy fetcha na mapie
9. Brzydko nazywasz paczki. Repozytorium nie należy do domeny biznesowej. Encje też. Natomiast obiekt biznesowy (Post, User) już tak. Tylko przypadkiem jest on jednocześnie DTOsem.
10. retrievePostsByUserId może wzrócić nulla i wtedy getPostsByUserIds walnie NPE
11. Nie iniektujesz poprawnego LocalDateTimeProvider w postsRepository
12. Osobiście przeniósłbym metodę getUser z PostFacade do UserFacade (tą która zwraca czystego Usera). I schował UserNotFoundException do paczki users. I schował ExceptionHandlerController (de facto UserExceptionHandlerController) do paczki users. Masz wtedy całą obsługę użytkowników zamkniętą w jednej paczce

Tyvrel napisał(a):

Jedziemy dalej:

1. w UserController brakuje ci <> przy ResponseEntity. Tego typu błędy wyłapuje IntelliJ, nie rozumiem czemu w ogóle to masz
2. A tak w sumie, to nie potrzebujesz ResponseEntity, tylko możesz zwrócić User i zaadnotować to @ResponseStatus(201). To samo w PostController

Racja, tak będzie lepiej

1. Czemu pola User nie są prywatne? Czemu obiekt nie jest immutable?

Przeoczenie, zapomniałem zmienić gdy usunąłem odnotację @Value

1. Twój UserEntity tylko udaje immutable, bo ma w polu mutable Set

Ok, chciałem mieć łatwiejszy sposób aktualizowania userów, ale może lepiej nadpisywać obiekty przy aktualizacji zamiast mutować

1. Mieszasz konfigurację za pomocą klas @Configuration i adnotacji nad klasami. Zdecyduj się na jedno rozwiązanie, żeby było konsekwentnie

Tego nie rozumiem, @Configuration jest ze springa, a adnotacje nad klasami z lomboka, nie używam annotation based configuration nigdzie.

1. Po co metoda mapUserEntityToUser zwraca funkcje, zamiast po prostu przyjąć UserEntity w parametrze? KISS
2. Chopuj buildery

Co to znaczy?

1. W InMemoryUsersRepository#trackUser user i userEntity to to samo =P. Zrobisz dwa razy fetcha na mapie
2. Brzydko nazywasz paczki. Repozytorium nie należy do domeny biznesowej. Encje też. Natomiast obiekt biznesowy (Post, User) już tak. Tylko przypadkiem jest on jednocześnie DTOsem.

Wzorowałem się na przykładzie Jakuba :)

1. retrievePostsByUserId może wzrócić nulla i wtedy getPostsByUserIds walnie NPE
2. Nie iniektujesz poprawnego LocalDateTimeProvider w postsRepository

Dzięki, przeoczenie.

1. Osobiście przeniósłbym metodę getUser z PostFacade do UserFacade (tą która zwraca czystego Usera). I schował UserNotFoundException do paczki users. I schował ExceptionHandlerController (de facto UserExceptionHandlerController) do paczki users. Masz wtedy całą obsługę użytkowników zamkniętą w jednej paczce

1. Hmm, w sumie racja. Używasz component scana tylko do controllerów
2. Chopowanie to sposób formatowania chainowanych metod

return PostEntity.builder().userId(post.getUserId()).postDate(getLocalDateNow()).message(post.getMessage()).build();
.
return PostEntity.builder()
		.userId(post.getUserId())
		.postDate(getLocalDateNow())
		.message(post.getMessage())
		.build();

W sumie nie podoba mi się to @Configuration. Ogranicza dostęp tylko do klas publicznych i z tego samego pakietu. A gdybym chciał wydzielić wiecej podpakietów musiałbym tworzyć odrębne klasy @Configuration dla każdego pakietu w którym mam niepubliczne klasy("beany"). Inaczej jak annotation based configuration który scanuje wszystko by default.