CR: REST API (Java, Spring)

Przejrzałem bardzo wyrywkowo. Zerknąłem do historii zmian. Commity nie są moim zdaniem dobrze opisywane. Np.:

Added getById to PostController // renamed MapperPhotogramException to InternalErrorPhotogramException // added dislikes field to Post entity // added mapping of liked and disliked posts to User entity // implemented PostServiceImpl // added isModerator, accessValidation, requireLogin to RoleService

I tak dalej. Komunikat wylicza, co się zmieniło w kodzie (dosłownie - co), a nie: po co, ani dlaczego. Taki opis jest zbędny (niepotrzebnie powiela informację, która i tak jest już siłą rzeczy zawarta w diffie) i mało funkcjonalny. Przeglądanie historii repozytorium - np. w poszukiwaniu czegoś konkretnego - gdzie wszystkie zmiany są opisane w taki sposób, to jest droga przez mękę.

Widzę też kilka innych wątpliwych praktyk. Np. metody w testach jednostkowych oznaczone jako private. A przed kim je "prywatyzujemy"? :) Po co zaśmiecać kod niepotrzebnymi słowami kluczowymi.

Testy nie są przy tym specjalnie czytelne, głównie ze względu na lakoniczne nazwy. Np. userEqualsWithId, w którym jedyną asercją jest assertEquals(getDefaultRole(), secondRole). To co w zasadzie jest tam weryfikowane? Bez wgłębienia się w kod mogę tylko zgadywać.

Rozważyłbym zastosowanie czytelnej struktury given / when / then - rozpoczynając od nazw testów. Ale także odnośnie przebiegu testów. Kiepską praktyką jest na przykład "rearanżacja" testu już po wykonaniu asercji:

assertTrue(user.isNew());
user.setId(ID);
assertFalse(user.isNew());

Tylko szybki rzut oka na testy:
https://github.com/tscript3r/photogram/blob/master/backend/src/test/java/pl/tscript3r/photogram/services/beans/CommentServiceBeanTest.java#L84
takie coś jak tutaj to dramat. Zrobiłeś test, który sprawdza czy implementacja wygląda tak jak w chwili kiedy pisałeś test. Nic więcej. No i trochę mocki potestowałeś.

https://github.com/tscript3r/photogram/blob/master/backend/src/test/java/pl/tscript3r/photogram/it/UserIT.java
To nie są testy integracyjne skoro lecisz tam MockMVC. Czemu nie wystartujesz http servera i nie poślesz prawdziwego requestu, żeby to serio przetestować?

Plus nazwy testów sa nieczytelne trochę. Dużo lepsza forma to should X when Y. Np. should return http client error when request with invalid data is sent zamiast loginAsNewUserWithoutEmailConfirmation. Bo twoja nazwa testu wcale nie mówi jasno co ten test sprawdza i czego oczekujemy.

W kwestii dobrych praktyk przy pisaniu testów polecam artykuły i prezentacje tego gościa: https://osherove.com/

Co do nazw: https://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html

Ma także (czy w zasadzie miał) dobry kanał na YT, gdzie m.in. robił dość pouczające "test review" przeglądając duże, open-source'owe projekty.

Na przykład:

1. Fajne Readme

2. Niefajny podział na pakiety. Podziel klasy na pakiety wedlug tego co robią, a nie tego czym są (czyli nie na pakiety service, controller itp tylko np photo, user)

3. Nie potrzebujesz interface jeśli masz tylko jedną implementacje

4. Trochę duże servce masz. Można je lekko porozbijać na mniejsze klasy. (Wbrew pozorom w springu można używać normalnych obiektów :D )

5. Jak już używasz Optionala to używaj, a nie po pierwszym pobraniu orElseThrow ;)

6. Nazywanie rzeczy bean itp. Staraj się cześć aplikacji gdzie jest logika maksymalnie odciąć od tego jakiego frameworku używasz. Nawet możesz same beany tworzyć gdzieś indziej (przez metody w jakiś klasach konfiguracyjnych). Ułatwia to niewpadanie w takie pułapki jak np dużo zależności jednej klasy.

7. Unikaj logiki w kontrolerach (sprawdzania isSet (btw, nazwa sugeruje czy cos jest Set a nie czy zostało podane ;) )

8. Ogarnij sobie moddelMapper, ewentualnie niech obiekt umie sam się przekonwertować do DTO.

9. Wraz z tym jak rośnie złożoność projektu warto odzielić klasy bazodanowe od domenowych. (Czyli np byłoby PostEntity jako takie troche DTO i Post z metodami biznesowymi)

10. Co do testów, dobrze jest móc postawić całą swoją aplikacje działającą w pamięci (również bez kontekstu springowego). Dzięki temu w ogole bedziesz mógł pominąć mocki, a działać na żywym orgamizmie ;)

A poza tym jeśli całość działa, to jest duży sukces jeśli jest to Twój pierwszy większy projekt ;) (Done is better then perfect)

@V-2: fakt, nigdy nie robiłem jakiś większych researchów na temat wiadomości w commitach, najwyższa pora.
Jeśli chodzi o private w metodach testowych dodałem je jedynie dlatego żeby intellij nie krzyczał (albo SonarLint, nie pamiętam), ponieważ najzwyczajniej w świecie mnie to drażniło, tym bardziej, że nie wydaje mi się by zrobiło to jakąś różnicę. Jeśli chodzi o testy - tutaj też raczkuję dopiero - nie poświęciłem najwidoczniej odpowiednio dużej ilości czasu na teorie i przyznam szczerze, że nie raz się gubiłem co mam testować, weryfikować - dlatego tam takie babole. Jeśli chodzi o zacytowany kod:

assertTrue(user.isNew());
user.setId(ID);
assertFalse(user.isNew());

jest dokładnie tym samym co:

user.setId(ID);
assertFalse(user.isNew());

Uznałem, że gdyby ze względu na jakieś zmiany domyślnie nowy user nie byłby nowy dodatkowa asercja by na to wskazała. Rozumiem co masz na myśli i jak wcześniej wspomniałem - zbyt mało teorii (po prostu preferuję praktykę). A podesłane przez Ciebie materiały z pewnością obadam, dzięki! :)

@Shalom: jak już powyżej wspomniałem - zbyt mało teorii przed praktyką jeśli chodzi o testy. Jeśli chodzi o testy integracyjne - to wynika stąd, że gdy chciałem je napisać wygooglowałem, znalazłem pierwszy 'lepszy' gotowiec na którym się wzorowałem i oto rezultat :) Wspominając jeszcze na temat nazewnictwa - tutaj też się zwyczajnie gubiłem jak je poprawnie nazywać. Dzięki!

@danek:
2. Jeśli chodzi o strukturę projektu wzorowałem się po prostu na na znalezionych na githubie projektach, jak również z kursów udemy. Przyznam szczerze, że nie spotkałem się z taką strukturą projektu (bądź nie zauważyłem zwyczajnie), w każdym razie przy następnym projekcie eksperymentalnie obadam jak wyjdzie :)
3. Tutaj to samo co wyżej - wzorowałem się innymi, sam w tym specjalnego sensu nie widziałem. Chociaż w założeniu miałem nieokreślenie daleką rozbudowę projektu, więc może jeszcze w niedalekiej przyszłości się to opłaci
4. Tak, kilkakrotnie w serwisach rzucam wyjątkami przy pomocy optionali na prawo i lewo - po prostu wygodne, mało kodu. A nie cierpię sprawdzać warunkami czy obiekt jest nullem, czy też nie, stąd takie ich zastosowanie. Generalnie w całym projekcie starałem się nie dopuścić do sytuacji gdzie funkcja zwraca null żeby tego nie robić
7. Znam zasadę, tutaj jednak się pokusiłem na wyjątek (trochę z lenistwa)
8. W poprzednium projekcie używałem MapStruct'a, z którego w ogóle zadowolony nie byłem, bo generował więcej problemów niż rozwiązywał, a ostatecznie większość mapowań sam musiałem nadpisać. Z pewnościa obadam modelMappera. Chociaż nie ukrywam - oprócz faktu, że musiałem markupowe interfejsy zastosować jestem bardzo zadowolony ze swojego rozwiązania.
9. Nie wiem jak - ale o tym sposobie i nie słyszałem i nie pomyślałem, przyda się! Miałem wielokrotnie rozkminę jak nie stworzyć niechcący hybrydy, chociaż tak bardzo byłoby to wygodne. Tylko czy w połączeniu z podpunktem 4 nie doprowadzi to do 'eksplozji' klas?

I tak - pierwszy, który bym jako większy określił. Dzięki! :)

Swoją drogą - na podstawie tego projektu czy jest to już poziom (pomijając testy, na których się teraz skupię) na którym mogę się powoli przymierzać do szukania pracy na juniora? A jeśli nie co powinienem jeszcze od strony praktycznej w ramach nauki wykonać?

tscript3r napisał(a):

1. Jeśli chodzi o strukturę projektu wzorowałem się po prostu na na znalezionych na githubie projektach, jak również z kursów udemy. Przyznam szczerze, że nie spotkałem się z taką strukturą projektu (bądź nie zauważyłem zwyczajnie), w każdym razie przy następnym projekcie eksperymentalnie obadam jak wyjdzie :)
2. Tutaj to samo co wyżej - wzorowałem się innymi, sam w tym specjalnego sensu nie widziałem. Chociaż w założeniu miałem nieokreślenie daleką rozbudowę projektu, więc może jeszcze w niedalekiej przyszłości się to opłaci

Niestety bo większość tutoriali tego uczy i potem żeby ogarnąc w projekcie co się dzieje trzeba skakać na lewo i prawo. Tutaj masz (nie idealny) przykład jak coś podobnego można osiągnąć

1. Tak, kilkakrotnie w serwisach rzucam wyjątkami przy pomocy optionali na prawo i lewo - po prostu wygodne, mało kodu. A nie cierpię sprawdzać warunkami czy obiekt jest nullem, czy też nie, stąd takie ich zastosowanie. Generalnie w całym projekcie starałem się nie dopuścić do sytuacji gdzie funkcja zwraca null żeby tego nie robić

Jak coś już siedzi w Optionalu to nie ruszasz tego. Jedyne co robisz to .map(), aż do momentu wypchania tego na front. Możesz to przeczytać ;)

1. Nie wiem jak - ale o tym sposobie i nie słyszałem i nie pomyślałem, przyda się! Miałem wielokrotnie rozkminę jak nie stworzyć niechcący hybrydy, chociaż tak bardzo byłoby to wygodne. Tylko czy w połączeniu z podpunktem 4 nie doprowadzi to do 'eksplozji' klas?

Osobiście wole mieć więcej mniejszych klas ;)

Swoją drogą - na podstawie tego projektu czy jest to już poziom (pomijając testy, na których się teraz skupię) na którym mogę się powoli przymierzać do szukania pracy na juniora? A jeśli nie co powinienem jeszcze od strony praktycznej w ramach nauki wykonać?

A próbowałeś na jakieś staże?

Dzięki wielkie za porady! Największym moim problemem w tym wszystkim jest fakt, że nie znam żadnego programisty, więc muszę trochę na ślepo działać gdy pojawiają się dylematy, a o porady pierwszy raz sięgnąłem.
Nie, nie próbowałem jeszcze - pracuję aktualnie w IT, z tym, że w zarządzaniu / administracji - mam już powoli tego dosyć stąd wróciłem do starej zajawki. Z racji, że praca da się znieść, zarobki są ok to się z tym nie spieszę. Tym bardziej, że zdaję sobie sprawę, że z początku pod kątem finansowym kolorowo to nie będzie.
Póki co to raczej myślałem o open source, przykładowo - elenx.net , ale to jak ogarnę swoje projekty do stopnia dla mnie zadowalającego.

tscript3r napisał(a):

@V-2: fakt, nigdy nie robiłem jakiś większych researchów na temat wiadomości w commitach, najwyższa pora.
Jeśli chodzi o private w metodach testowych dodałem je jedynie dlatego żeby intellij nie krzyczał (albo SonarLint, nie pamiętam)

To bym sobie wyłączył tę inspekcję, i tyle. Głupie IDE nie ma w tym wypadku racji.

Kopiuj

assertTrue(user.isNew());
user.setId(ID);
assertFalse(user.isNew());

jest dokładnie tym samym co:

Kopiuj

user.setId(ID);
assertFalse(user.isNew());

Uznałem, że gdyby ze względu na jakieś zmiany domyślnie nowy user nie byłby nowy dodatkowa asercja by na to wskazała.

...i wtedy test nie przetestuje tego, co sprawdza druga asercja, bo już się wywalił :) Tak że nie, nie jest "dokładnie tym samym".

Problem z tą praktyką polega na tym, że jeden test ma teraz różne możliwe powody do wywalenia się. Dowiadując się, że któryś nie przeszedł - nie jesteś już w stanie powiedzieć po samej nazwie, co wywołało błąd.

Tak że kiedy jakieś zmiany wywołają buga, który zepsuje oba testowane zachowania naraz - czyli nowy użytkownik nie będzie miał flagi isNew(), a zmiana danych ją włączy, zamiast wyłączyć - test tego nie wykryje. Test wywali się po wykryciu pierwszego problemu, a o istnieniu drugiego dowiesz się dopiero, gdy już naprawisz pierwszy.

To poważna zmiana (gdyby takie podejście było w naszych testach normą). Testy nie dają nam już jasnego obrazu, w jakim stanie jest system. Informują nas po kawałeczku.

A wystarczy rozbić to na dwa osobne testy, skoro przecież testujesz dwa różne zachowania - i ten problem przestaje istnieć.

Nie jestem purystą, który uważa, że jeden test = jedna asercja, to być może przesada... ale jeden test powinien testować jeden koncept, jedno zachowanie. A seria asercji nie powinna już być przetykana niczym innym.

Rozumiem co masz na myśli i jak wcześniej wspomniałem - zbyt mało teorii (po prostu preferuję praktykę).

Nie ma nic praktyczniejszego niż dobra teoria ;) Wszystko, co wyżej, opiera się o względy praktyczne.