Prośba o Code Review

0

Cześć. Piszę do was z prośbą o ocenę kodu mojej aplikacji. Pisałem ją przez 4 wieczory i 3 dni od zera. Jest to mój pierwszy "większy" projekt w Springu i zdaję sobie sprawę, że to tylko prosty CRUD bez logiki. Pisząc go chciałem podsumować swoją wiedzę przed przejściem do nauki REST`ów i przy okazji stworzyć jakiś projekt do portfolio, które chciałbym ewentualnie kiedyś wykorzystać do znalezienia pierwszego stażu wakacyjnego. Planuję w najbliższym wolnym czasie popracować nad Security ( dodawanie użytkowników, edycja ról itp. ). Prosiłbym o skupienie się na samym backendzie, ponieważ frontend u mnie raczkuje. Z góry dziękuję za pomoc, rady, konstruktywną krytykę i poświęcony czas.

Link do repozytorium : https://github.com/DSniatecki/YourFleetManager

1

Tak poza tym to na oko wygląda ok, CRUD jak CRUD, w zasadzie nie ma tam funkcjonalnego kodu, więc trudno cokolwiek więcej powiedzieć.
Mógłbyś przynajmniej dopisać tam jakieś "prawdziwe" testy, tzn takie które wystartują ci jakąś bazę in memory i postawią aplikacje na jakimś embedded jetty/tomcat i faktycznie będziesz stukać po endpointach.

0
Shalom napisał(a):

Poprawione.

Zmienione na "entities".

Tak poza tym to na oko wygląda ok, CRUD jak CRUD, w zasadzie nie ma tam funkcjonalnego kodu, więc trudno cokolwiek więcej powiedzieć.
Mógłbyś przynajmniej dopisać tam jakieś "prawdziwe" testy, tzn takie które wystartują ci jakąś bazę in memory i postawią aplikacje na jakimś embedded jetty/tomcat i faktycznie będziesz stukać po endpointach.

Dzięki za rzucenie okiem i wszystkie rady :)

1

Na twoim miejscu pozmieniałbym nazwy testów. W JUNIT 5 masz do dyspozycji adnotację @DisplayName dzięki której możesz stworzyć czytelną nazwę testu. Niektóre testy mają tytuł totalnie nie adekwatny do tego co dany test robi.

Moim zdaniem powinieneś dodać sobie jakąś baze inMemory np H2 i koniecznie stworzyć testy integracyjne dla kontrolerów, chociażby. Dobrą praktyką jest też odpowiednie nazewnictwo klas testów tzn dla testów jednostkowych dodawać suffix UnitTest lub UTest lub po prostu UT, dla integracyjnych IntegrationTest lub ITest lub IT

czyli np UserControllerITest, UserServiceUTest itd.

0
witold12 napisał(a):

Na twoim miejscu pozmieniałbym nazwy testów. W JUNIT 5 masz do dyspozycji adnotację @DisplayName dzięki której możesz stworzyć czytelną nazwę testu. Niektóre testy mają tytuł totalnie nie adekwatny do tego co dany test robi.

  • Początkowo chciałem napisać testy jednostkowe właśnie w JUnit 5, ale nie mogłem znaleźć sposobu jak "zmockować" komponenty do testów by później przy nich nie był wczytywany kontekst, a w JUnit 4 wyszło mi to bez problemu.

Moim zdaniem powinieneś dodać sobie jakąś baze inMemory np H2 i koniecznie stworzyć testy integracyjne dla kontrolerów, chociażby. Dobrą praktyką jest też odpowiednie nazewnictwo klas testów tzn dla testów jednostkowych dodawać suffix UnitTest lub UTest lub po prostu UT, dla integracyjnych IntegrationTest lub ITest lub IT

czyli np UserControllerITest, UserServiceUTest itd.

  • Dzięki za rady. W wolnym czasie przerobię te testy od początku tak jak trzeba.

Mam jeszcze kilka pytań o testy. Jak to jest z tym TDD ? Dużo osób to praktykuje ? Jeśli nie praktykujecie TDD, to w którym momencie piszecie testy ? Czy takie pisanie testów do działającego już kodu ma sens ?

1

Tam gdzie się da staram się pisać testy przed kodem, bo przynajmniej mam jakąś wizję jak coś ma działać i wiem czy działa (szybciej tak sprawdzać niż ręcznie). Pisanie testów do działającego kodu ma taki sens, że na własnej skórze odczujesz co to znaczy nietestowalny kod :P

0

A jak rygorystyczne mają być testy ? Każda metoda ma być przetestowana dla każdej sytuacji ? Czyli osobny test dla sukcesu i osobny dla każdego możliwego wyjątku przez nią rzuconego ? Jeśli np. 2 metody rzucają ten sam wyjątek w serwisie dla tej samej sytuacji to powinno się napisać 2 osobne testy dla nich, sprawdzające czy każda z nich rzuca ten wyjątek ? Sorka za zasypywanie pytaniami :D

1
DamianSn napisał(a):

A jak rygorystyczne mają być testy ? Każda metoda ma być przetestowana dla każdej sytuacji ?

To zależy. Krytyczne częsci projektu powinny być przetestowane pod każdym możliwym względem (krytyczne-> te które faktycznie zarabiają pieniądze, ewentualnie kwestie bezpieczeństwa). Reszta natomiast zazwyczaj starczy jakiś happy path plus jakieś typowe przypadki skraje, bez jakiegoś cudowania.

Czyli osobny test dla sukcesu i osobny dla każdego możliwego wyjątku przez nią rzuconego? Jeśli np. 2 metody rzucają ten sam wyjątek w serwisie dla tej samej sytuacji to powinno się napisać 2 osobne testy dla nich, sprawdzające czy każda z nich rzuca ten wyjątek? Sorka za zasypywanie pytaniami :D

Nie rzucaj wyjątkami ;) Zależy jak różne są to sytuacje, ale najlepiej chociaż po teście na wyjątek.

Tu masz przykładowe z jakiegoś mojego pobocznego projektu. Może się jakoś nimi zasugerujesz ;)

1

Każda metoda ma być przetestowana dla każdej sytuacji ?

Tego w ogólnym przypadku nigdy nie osiągniesz (dla niektórych można zrobić przegląd przez wszystkie możliwe wartości, np. jeśli używasz 32-bitowego inta), ale istnieje coś takiego jak property testing gdzie opisujesz jakie właściwości ma mieć funkcja na uproszczonym modelu. W czasie testów biblioteka do property testingu generuje losowe wartości i sprawdza czy cechy, które są wymagane są spełnione. Przykładowy zbiór testów dla funkcji dodawania (pseudo składnia inspirowana Elixirem):

check all a <- integer(), b <- integer(), c <- integer() do
  assert a + b == b + a # przemienność
  assert a + 0 == a     # element neutralny
  assert (a + b) + c == a + (b + c) # łączność
end

W ten sposób jesteś w stanie często całkiem szybko wyłapać trochę mniej oczywiste błędy.

0

Siemaneczko. Przez ostatnie dwa dni stworzyłem RESTową, ulepszoną wersję tej aplikacji. Poprawiłem testy jednostkowe i przeskoczyłem na JUnit5. Niestety ciągle nie napisałem testów integracyjnych. Jest to w sumie moja pierwsza REST`owa apka ( nie licząc restowego hello worlda ). Co ja się namęczyłem z MapStructem to szkoda gadać, a na końcu i tak część mapperów sam napisałem . Jak będziecie mieli ochotę to możecie rzucić okiem. Rady i konstruktywna krytyka zawsze mile widziane. Z góry dzięki !

Link do repo : https://github.com/DSniatecki/YourFleetManager.V2-REST

Przy okazji mam dwa pytanka :

  • czy metoda HTTP POST powinna zezwalać na edycję obiektu, czy zawsze ma dodawać go jako nowy ?
  • czy poza zwróceniem responsa OK DELETE powinien coś robić w przypadku sukcesu ?
0

Od edycji jest PUT

0
Gworys napisał(a):

Co ty w ogóle testujesz.? https://github.com/DSniatecki/YourFleetManager.V2-REST/blob/master/src/test/java/com/dsniatecki/yourfleetmanager/controllers/CarControllerUnitTest.java
Ja tam widzę mocka który operuje na drugim mocku.

  • Prosty test jednostkowy, czy poprawnie zachodzi proces serializacji do JSON`a i czy jest dobry status,

Po co ci ten interfejs ? https://github.com/DSniatecki/YourFleetManager.V2-REST/tree/master/src/main/java/com/dsniatecki/yourfleetmanager/services/car

  • Staram się korzystać z Dependency Injection do interfesjów. Wydaje mi się, że jest do dobra praktyka.

To nie są żadne obiekty domenowe zwane potocznie "encjami". No chyba, że twoją domeną jest relacyjna baza danych.
https://github.com/DSniatecki/YourFleetManager.V2-REST/tree/master/src/main/java/com/dsniatecki/yourfleetmanager/entities

  • To jak to poprawnie nazwać ? Założyłem, że skoro coś jest oznaczone przez adnotacje @Entity to jest to encja. Korzystam z relacyjnej bazy danych, ale nie jest to moja domena.

Nie da się tego jakoś lepiej napisać w Javie ?
https://github.com/DSniatecki/YourFleetManager.V2-REST/blob/master/src/main/java/com/dsniatecki/yourfleetmanager/mappers/car/CarPartialMapper.java

  • Tego akurat wydaje mi się, że nie. Jest to specjalnie tak napisane by przypisywało tylko co nie jest nullem. Dzięki temu jak przekaże tylko część parametrów w JSON`ie to tylko ta część zostanie zmieniona. Taki był mój cel.

No i całość to oczywiście przepyszna lazania.

  • Dlaczego? Wydaje mi się, że wszystko idzie według dobrego schematu i na siebie nie nachodzi. Controller odpiera DTOSy > Service mapuje > Repo zapisuje. Jest wiele DTOsów, co mogło popsuć czytelność, ale schemat jest chyba w miarę dobry.

Ciągle się uczę i dzisiaj użyłbym projekcji, trochę pozmieniał te testy, częściej tworzył i korzystał z builderów. Czytelność jest dla mnie zazwyczaj priorytetem. Jeśli coś jest źle to chętnie to poprawię, ale chciałbym zrozumieć ten błąd. Trochę nie rozumiem tego negatywnego podejścia, ale i tak dzięki. Super byłoby gdybyś wytłumaczył trochę bardziej te błędy, ale nic nie wymagam :)

2

DI nie polega na tworzeniu interface a na wrzucaniu zależności z zewnątrz (przez konstruktor czy jak tam zwał). Jak masz mieć tylko jedną implementacje to nie ma sensu mieć do tego interface

1

Prosty test jednostkowy, czy poprawnie zachodzi proces serializacji do JSON`a i czy jest dobry status,

Testowanie mocka z mockiem wydaje mi się tak samo bez sensu, jak rzucanie wyjątku, kiedy Optional jest "non'em".

     private void checkIsNotPresent(Optional<Company> optional, Long id){
        if(!optional.isPresent()){
            throw new ResourceNotFoundException("Company[id:" + id +"] was not found.");
        }

To jak to poprawnie nazwać ? Założyłem, że skoro coś jest oznaczone przez adnotacje @Entity to jest to encja. Korzystam z relacyjnej bazy danych, ale nie jest to moja domena.

A no fakt jest tam logika biznesowa. :)

    public void addCar(Car car){
        if(this.cars == null) this.cars = new ArrayList<>();
        car.setDepartment(this);
        this.cars.add(car);
    }

Wiec nie można powiedzieć, że nie jest to DM.

Ja bym wolał po prostu operować na presistance objects z DAO. No, chyba że ta warstwa domenowa to jakaś inwestycja na przyszłość.

Tego akurat wydaje mi się, że nie. Jest to specjalnie tak napisane by przypisywało tylko co nie jest nullem. Dzięki temu jak przekaże tylko część parametrów w JSON`ie to tylko ta część zostanie zmieniona. Taki był mój cel.

Nie można tam użyć Optionala z 'Map'.?

Dlaczego? Wydaje mi się, że wszystko idzie według dobrego schematu i na siebie nie nachodzi. Controller odpiera DTOSy > Service mapuje > Repo zapisuje. Jest wiele DTOsów, co mogło popsuć czytelność, ale schemat jest chyba w miarę dobry.

Tworzenie modułów per nazwa wzorca lub czegoś, co nawiązuje do używanego języka jak np. "Interfaces" zawsze jest złe.
Najpierw zawsze powinno występować powiązanie logiczne modelu.
np.
Departaments
---DepartamentSrvice
---DepartamentDto
---Cars
------CarsDto
------CarsService

Nie musisz też wszędzie używać nazw 'Service' często zabija to kreatywność i pogarsza czytelność modelu. Czasem lepiej jest napisać to co ta usługa robi.
np. Zamiast BoxService możesz napisać PackingBox.

1

Dzięki za wytłumaczenie i wiele cennych uwag. W wolnym czasie wezmę się za poprawianie tej lazanii :)

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