Code Review - serwerowa część aplikacji do nauki słówek

Cześć, w ramach rozwijania swoich umiejętności tworzę projekt w postaci aplikacji do nauki słówek (fiszki). Część serwerowa aplikacji napisana jest w springu, postanowiłem wykorzystać w tym przypadku rest api wraz z uwierzytelnianiem użytkowników za pomocą tokenów JWT. Projekt jest w fazie rozwojowej, mam jeszcze parę rzeczy do zrobienia, żeby jeszcze bardziej poszerzyć swoją wiedzę, jednak już teraz chciałbym zasięgnąć małego codereview od osób, które mają już doświadczenie w javie.

projekt na github: link

Głównie chciałbym poznać waszą opinię na temat testów, na razie są tylko dwa, ponieważ staram się nauczyć podejścia do testów oraz korzystania z mockito. Czy są one dobrze przeprowadzone, czy jednak trzeba je gruntownie poprawić.
Poza tym mam wrażenie, że zagnieżdżenie paczek jest trochę niespójne i porozrzucane. Chciałem zabrać się za refactoring tego już wcześniej, jednak wstrzymałem się aż do tego postu, żeby poznać waszą opinię. Przyjąłbym też chętnie uwagi odnośnie nazewnictwa, na przykład przy serwisach, gdzie starałem się nie używać skrótu "Impl", a zamiast tego na przykład "General...".
Wiem też, że do poprawy są kody zwracane przez kontrolery, na razie poustawiałem je tak w ramach testów przez postmana.

Wszelkie inne uwagi również mile widziane, mam nadzieję, że nie będzie aż tak źle i nie trzeba będzie pisać tego projektu od początku.

Weźmy na tapet pierwszy test:

 @Test 
     public void shouldFindAllFlashcard() throws Exception { 
         when(flashcardService.findAll()).thenReturn(getFlashcards()); 
 
 
         List<Flashcard> flashcards = (List<Flashcard>) flashcardService.findAll(); 
 
 
         assertEquals(2, flashcards.size()); 
         assertEquals((Object) 1L, flashcards.get(0).getId()); 
         assertEquals((Object) 2L, flashcards.get(1).getId()); 
     } 

Najwpierw mockujemy flashcardService.findAll i za opdowiedź podstawiamy coś tam (zdefiniowane niżej)

 private List<Flashcard> getFlashcards() { 
       List<Flashcard> flashcards = new ArrayList<>(); 
 
 
         flashcard.setId(1L); 
        flashcards.add(flashcard); 
 

         Flashcard flashcard2 = new Flashcard(); 
         flashcard2.setId(2L); 
         flashcards.add(flashcard2); 

 
         return flashcards; 
     } 
 } 

Potem asercjami testujesz czy to zwrócono:

         List<Flashcard> flashcards = (List<Flashcard>) flashcardService.findAll(); 
 
 
         assertEquals(2, flashcards.size()); 
         assertEquals((Object) 1L, flashcards.get(0).getId()); 
         assertEquals((Object) 2L, flashcards.get(1).getId()); 

I pewnie - o niespodzianka ! - się wszystko zgadza.

Pytanie: Co ten test testuje?

Odpowiedź:
Ten test testuje Mockito. W imieniu Szczepana Fabera - dziękuję ( To będzie milion któryś taki test Mockito)

Pobieżny przegląd pokazuje, że wszystkie twoje testy testują Mockito :-) (i tylko Mockito).

Poza tym kod ogólnie całkiem OK jak na Springa :-)
Nieprzeinżynierowana wersja standardowej architektury korporacyjnej nr 1. Łatwo się w tym kodzie ogarnąć.

Trudno się nie zgodzić z powyższym. Gdyby ten test miał mieć jakikolwiek sens to należałoby mockować przynajmniej FlashcardRepository. Niemniej nadal taki test ma znikomą wartość i niewielki sens bo przecież ty w tym swoim "serwisie" tylko delegujesz wywołania metod do repozytorium, które jest generowane przez Spring-Data. Więc w zasadzie co chcesz testować? Czy x.metoda() faktycznie wywoła metodę na x? To prawie jak testowanie getterów/setterów. Niby można, ale nie wiadomo po co.

Zanim napiszesz test zastanów się co chce przetestować?.

Nie przyglądałem się mocno całej aplikacji ale jedno mi się rzuciło w oczy. Jaki jest sens używania w controllerach wszędzie gdzie się da ResponseEntity nawet jeżeli faktycznie nie korzystasz z tego co Ci to daje tylko zawsze zwracasz np. coś takiego: new ResponseEntity<Deck>(createdDeck, HttpStatus.OK)?

@Corriel: generalnie na początku miałem inny zamysł architektury i w niektórych miejscach ustawiałem jeszcze headers. Zostawiłem je tak, ponieważ nauczyłem się takiego podejścia. Istnieje jakieś korzystniejsze rozwiązanie, gdzie można dodatkowo określić jaki kod response ma być zwracany?

@Shalom @jarekr000000 duża część tego projektu opiera się na tym co oferuje spring sam w sobie, Kiedyś czytałem, że testowanie kontrolerów też nie ma zbytnio sensu, więc co pozostaje przetestować, wyłącznie klasy "utils"?

@chuuck testuje się LOGIKĘ aplikacji. Jeśli aplikacja to generyczny CRUD to nie ma czego testować. Ba, nie ma nawet sensu takiej aplikacji pisać, bo taki Spring Roo może ją zwyczajnie wygenerować jak mu tylko podasz listę tabel do bazy i listę pól. Ba, nawet formatki ci sam wygeneruje do tego i logowanie skonfiguruje.

Jak już przejdziesz do pisania aplikacji która COŚ ROBI to wtedy nagle będzie wystarczająco dużo serwisów i obiektów biznesowych do przetestowania. Co aplikacja moze robić? Na przykład sterować marsjańskim łazikiem albo sterować trajektorią lotu nuklearnego pocisku balistycznego. I nie, zapewniam cię że nie będzie tam klasy trajektoriaPociskuUtils bo miałaby miliony linii ;]

Powiedzmy, że twój **RegistrationController ** minimalną logikę ma. Więc rejestrację możesz przetestować.

kilka istotnych tematow na ktore warto zwrocic uwage poza tym co panowie wyzej podali,

• nie potrzebujesz w encjach do kazdego pola getterow i setterow, ustaw access na @access(FIELD) , encja niech zawiera kilka metod ktore wynikaja z jej natury, mniejszy burdel bedziesz mial, dopiero jak sie okaze ze potrzebujesz , wtedy bedziesz dodawał konkretne gettery
• nie wiem czy konwencja nazewnicza tabel w bazach SQL w stylu jak u ciebie @Table(name = "BasicUser") jest porządana. tutaj czesto stosuje sie nazewnictwo w stylu BASIC_USER, jak nie podasz w ogóle nazwy a JPA sam za ciebie z tego co kojarze uzyje wlasnie czegos podobnego
• kolejne pytanie czy encja BasicUser potrzebuje nienaturalnego klucza w postaci Long id ? Mas przeciez username ktory prawdopodobnie bedzie unikalny i moze byc identyfikatorem, czyz nie ? Rownie dobrze to adres email moze byc identyfikatorem a username miec uniq constraint albo odwrotnie
• dziwna sprawa dla mnie jest taka ze np service BasicUserServcie zawiera metode o nazwie post ktora robi tak naprawde save'a, skad taka nazwa ?
• w metodzie patch wykonujesz update(basicUser), jelsi encja jest aktualnie zwiazana z contextem, jest zarzadzalna, nie potrzebujesz wywolywac metody update, bo i tak na koniec transkacji kontener wykryje kazda zmiane na encji i sam wykonane update pod spodem
• tak samo nie ma potrzeby aby metoda patch zwracała obiekt ktory updatujesz, po co dodatkowy call do bazy skoro juz masz encje ta ktora updatujesz?
• metoda delete w tym samym serviceie zwraca true ? po co ? skoro masz tam dwie opcje albo poleci wyjatek albo, jak nie poleci to po co ci wartosc true ?
• nie wiem w jakiej javie pisze ale uzywanie import java.util.Date jest ZLE, w javie 8 masz nowe api dla dat, ewentualnie uzyj Joda-time
• metoda GeneralFlashcardService.update jest napisana do kitu
• z tym niektorzy moga sie kłócic ale czy tworzenie interfejsu do kazdego service'u do którego prawdopodbnie i tak niegdy nie napisze innej implementacji ma sens, dla mnie niekoniecznie
• w pakiecie model trzymasz model, repository i service
• za czesto poslugujesz sie id, np metoda w kontrolerze przyjmuje Long id, pozniej robisz getById, a moteda w kontrolerze moze przyjmowac odrazy Encje, musisz tlyko napisac konwerter , koda staje sie czystszy i bardziej przejrzysty: http://geekabyte.blogspot.com/2014/10/using-springs-type-converter-to-inject.html
• do testowania proponuje dodac spock'a, testy sa duzo bardziej przejrzyste

szkoda ze nie mozna komentowac na twoim githubie, bo sporo rzeczy masz do poprawienia jak dla mnie, dodatkowo programujesz dosc defensywanie

@kemot:

z tym niektorzy moga sie kłócic ale czy tworzenie interfejsu do kazdego service'u do którego prawdopodbnie i tak niegdy nie napisze innej implementacji ma sens, dla mnie niekoniecznie

To nie jest do końca taka prosta sprawa bo Spring przez długi czas miał problem (może nadal ma?) z proxowaniem klas dla AOP. Wymagało to cgliba i dodatkowej konfiguracji. Domyślnie jeśli zrobiłeś @Transactional dla metody która nie pochodzi z interfejsu to zwyczajnie nie działało, bo spring nie mógł wygenerować class-proxy a interface-proxy nie było możliwe, bo interfejsu nie było. Więc nie jest to do końca bez sensu mimo wszystko. Dodatkowo ułatwia to testowanie bo mozesz sobie wrzucić do testu własną implementacje dummy jakiegoś serwisu, bez konieczności mockowania, co może być ułatwieniem. Szczególnie jeśli trzeba ustawić np. zachowanie dla wielu metod takiego serwisu. Czytelniej to wygląda jak masz po prostu implementacje dummy z jednolinikowymi metodami niz jak masz 20 linijek ustawiania zachowania mocka w teście.

Shalom napisał(a):

@kemot:

z tym niektorzy moga sie kłócic ale czy tworzenie interfejsu do kazdego service'u do którego prawdopodbnie i tak niegdy nie napisze innej implementacji ma sens, dla mnie niekoniecznie

To nie jest do końca taka prosta sprawa bo Spring przez długi czas miał problem (może nadal ma?) z proxowaniem klas dla AOP. Wymagało to cgliba i dodatkowej konfiguracji. Domyślnie jeśli zrobiłeś @Transactional dla metody która nie pochodzi z interfejsu to zwyczajnie nie działało, bo spring nie mógł wygenerować class-proxy a interface-proxy nie było możliwe, bo interfejsu nie było. Więc nie jest to do końca bez sensu mimo wszystko. Dodatkowo ułatwia to testowanie bo mozesz sobie wrzucić do testu własną implementacje dummy jakiegoś serwisu, bez konieczności mockowania, co może być ułatwieniem. Szczególnie jeśli trzeba ustawić np. zachowanie dla wielu metod takiego serwisu. Czytelniej to wygląda jak masz po prostu implementacje dummy z jednolinikowymi metodami niz jak masz 20 linijek ustawiania zachowania mocka w teście.

co do springa,wiem o czym mówisz, dluzszy czas tych problemow nie mialem lae rzeczywiscie cos w tym stylu było,
oczywiscie interfejsy sa porzadane, dawno nie zdarzyło mi sie napisal wlasnej implemnetacji mocka, raczej korzystam z dobrodziejstw spocka czy tez mockito i czuje sie z tym nienajgorzej dlatego nie pisze interfejsow jesli tego nie wymaga konwencja w projekcie lub po prostu rozwiazanie ktore uzywam, ale oczywiscie interfejsy powinny byc wykorzystywane tylko ja widze ich uzycie zgodnie z zasada interface segragation, a takie servicy to zazwyczaj wychodza fat interfacy takie ze hoho i wtedy sa uzywane tlyko po to zeby obejsc nieudolnosci springowe badz latwiej zamockowac klase w testach