Code review i wskazówki jak poprawnie pisać testy

0

Cześć,

napisałem pewien projekt i chciałbym sprawdzić jakiej jest jakości. Aktualnie jestem na etapie pisania testów.
Liczę na waszą konstruktywną krytykę, czepiajcie się wszystkiego (no może bez czepiania się, że projekt jest napisany w Javie :P).
Docelowo projekt ma mieć ponad 80% pokrycia testami (najlepiej 100%).

Link - Jshorten

O projekcie:
Jest to prosty link shortener napisany przy użyciu Spark framework. Docelowo ma to być rozwiązanie, które ktoś może w łatwy i szybki sposób zmodyfikować do swoich potrzeb i uruchomić.

Z niemałym trudem podpiąłem pod repo:

  • Travis
  • SonarQube
  • Codecov

dzięki czemu zmiany są widoczne na bieżąco.
Projekt jest dostępny dzięki Heroku po adresem - jshorten-heroku.
Może chwilę wstawać bo jest na darmowym dyno, które po 30 minutach nieaktywności usypia aplikację.

Wszelkie uwagi można wpisywać tutaj i jako issue na repo (gdyby ktoś miał ochotę).

Dzięki!

2

Shortening* a nie shorting :)

1
  1. Rażą takie długie lambdy zamiast method refence.
    2)Po co interfejsy z jedną implementacją (chyba ze rzeczywiście możesz potrzebować podmieniac implementacje)
    3)Radzę robić klasy i pola jako final (chyba że klasa jest przeznaczona do dziedziczenia)
0

@scibi92:

  1. Rażą takie długie lambdy zamiast method refence.

Słuszna uwaga, do poprawy.

2)Po co interfejsy z jedną implementacją (chyba ze rzeczywiście możesz potrzebować podmieniac implementacje)

Docelowo ma to być rozwiązanie, które ktoś może w łatwy i szybki sposób zmodyfikować do swoich potrzeb i uruchomić. Natomiast chcę, aby osoba, która skorzysta z tego kodu miała wolność wyboru - może wybrać domyślną implementację lub stworzyć własną.

3)Radzę robić klasy i pola jako final (chyba że klasa jest przeznaczona do dziedziczenia)

Większość klas jest do dziedziczenia, niektóre pola nie, również celna uwaga.

P.S. Mam nadzieję, że nie wypaliło oczu ;)

4

No dobra, to patrze sobie na testy a tam:

  1. https://github.com/Burdzi0/Jshorten/blob/master/src/test/java/shortener/url/service/IllegalTimestampExceptionTest.java albo https://github.com/Burdzi0/Jshorten/blob/master/src/test/java/shortener/url/service/validator/ValidationExceptionTest.java
    Przepraszam ale co dokładnie sprawdza ten test? Czy throw działa w javie? Czy assertThrows działa? o_O
  2. https://github.com/Burdzi0/Jshorten/blob/master/src/test/java/shortener/url/service/DefaultUrlServiceImplTest.java#L45
    Jestem fanem pisania małych, jasnych testów z jasnymi nazwami, tzn shouldThrowExceptionWhenEmpty/Null/WhitespaceUrlIsPassed cz coś ;] Teraz masz tam masę testów które w zasadzie nie wiadomo co dokładnie testują i jak się mają zachować. Nic mi nie mówi test save albo createUrlValidationException (czemu nie shouldThrowValidationExceptionForMalfomedUrl?)
  3. https://github.com/Burdzi0/Jshorten/blob/master/src/test/java/shortener/url/service/DefaultUrlServiceImplTest.java#L101 może Awaitility zamiast tego? Ma fajną składnię i jest bardziej czytelne niż taki losowy sleep
  4. https://github.com/Burdzi0/Jshorten/blob/master/src/test/java/shortener/url/service/factory/DefaultUrlFactoryTest.java#L47 testy setterów i getterów, moje ulubione :D
  5. https://github.com/Burdzi0/Jshorten/blob/master/src/main/java/shortener/url/handler/DefaultDuplicateHandlerImpl.java#L7 nie rozumiem sensu tej klasy trochę. Czemu to nie jest część obiektu domenowego UrlPojo tylko jakaś funkcja wyciągnięta na zewnątrz która miesza tam w środku obiektu i mu zmienia jakieś pola?
  6. https://github.com/Burdzi0/Jshorten/blob/master/src/main/java/shortener/url/Server.java#L71 może Builder? ;)
  7. Wszędzie brakuje final przy polach klas i nie jest to generalnie thread-safe (ten InMemoryRepo na przykład)
  8. Widząc te wszystkie UrlFactory i UrlCreatory czuje się jak czytając enterprise foo-bar jakis :D
  9. https://github.com/Burdzi0/Jshorten/blob/master/src/main/java/shortener/url/algorithm/Sha256ShortingAlgorithm.java#L33 jak zawsze lepiej dać taki komentarz, niż np. wydzielić metodę bytesToHex, prawda? :)
0

@Burdzi0:
własnie, InMemoryRepository powinno używać ConcurrentHashMap zamiast HashMap.
I czemu w modelu massz settery zamiast robić model niemutowalny?

0

https://github.com/Burdzi0/Js[...]alTimestampExceptionTest.java albo https://github.com/Burdzi0/Js[...]/ValidationExceptionTest.java
Przepraszam ale co dokładnie sprawdza ten test? Czy throw działa w javie? Czy assertThrows działa? o_O

oraz

https://github.com/Burdzi0/Js[...]efaultUrlFactoryTest.java#L47 testy setterów i getterów, moje ulubione :D

Podpiąłem SonarQube i Codecov i właśnie te narzędzia pokazały mi, że np. konstruktory nie są pokryte testami. @Burdzi0 niewiele myśląc, chcąc być w przyszłości dobrym programistą, wziął się do pracy nie zastanawiając się czy jest tego sens :P Rzeczywiście trochę pofrunąłem (to samo z getterami i setterami). Czy takie części kodu wyklucza się z tego typu narzędzi? Nie zwraca uwagi, że pokrycie testami nie jest całkowite czy jak?

https://github.com/Burdzi0/Js[...]ltUrlServiceImplTest.java#L45
Jestem fanem pisania małych, jasnych testów z jasnymi nazwami, tzn shouldThrowExceptionWhenEmpty/Null/WhitespaceUrlIsPassed cz coś ;] Teraz masz tam masę testów które w zasadzie nie wiadomo co dokładnie testują i jak się mają zachować. Nic mi nie mówi test save albo createUrlValidationException (czemu nie shouldThrowValidationExceptionForMalfomedUrl?)

Bardzo słuszna uwaga, korzystałem z metod wygenerowanych przez IntelliJ, poszedłem na skróty. Zapisane do poprawy.

https://github.com/Burdzi0/Js[...]tUrlServiceImplTest.java#L101 może Awaitility zamiast tego? Ma fajną składnię i jest bardziej czytelne niż taki losowy sleep

Nigdy nie słyszałem, na pewno zmienię.

https://github.com/Burdzi0/Js[...]efaultUrlFactoryTest.java#L47 testy setterów i getterów, moje ulubione :D
https://github.com/Burdzi0/Js[...]tDuplicateHandlerImpl.java#L7 nie rozumiem sensu tej klasy trochę. Czemu to nie jest część obiektu domenowego UrlPojo tylko jakaś funkcja wyciągnięta na zewnątrz która miesza tam w środku obiektu i mu zmienia jakieś pola?

Sposób w jaki dany url jest skracany może być różny (przynajmniej takie jest założenie). Chciałem, aby to rozwiązanie było możliwie elastyczne. Jako, że sam UrlPojo (czyli przykładowy model) nie zajmuje się skracaniem linku, to nie powinien zajmować się rozwiązywaniem problemu zaistniałych duplikatów. Ciężko mi tutaj ustalić kto powinien być właścicielem tej funkcjonalności (co zapewne wynika z tragicznej architektury tego mini projektu). Jakieś wskazówki?

https://github.com/Burdzi0/Js[...]shortener/url/Server.java#L71 może Builder? ;)

Czasami sam siebie nie rozumiem, tutaj to się aż prosi :P

Wszędzie brakuje final przy polach klas i nie jest to generalnie thread-safe (ten InMemoryRepo na przykład)

Rzeczywiście, nie zwróciłem uwagi na dostęp wielowątkowy, zapisane do poprawy.

Widząc te wszystkie UrlFactory i UrlCreatory czuje się jak czytając enterprise foo-bar jakis :D

Chodzi tutaj o nazewnictwo czy architekturę? Przerost formy nad treścią?

https://github.com/Burdzi0/Js[...]256ShortingAlgorithm.java#L33 jak zawsze lepiej dać taki komentarz, niż np. wydzielić metodę bytesToHex, prawda? :)

< chowa twarz w dłonie > - będzie poprawione.
Bardzo dziękuję za poświęcony czas :) to naprawdę dużo informacji dla mnie. Zwracam pieniądze za krople na krwawiące oczy!

@Pinek

https://github.com/Burdzi0/Js[...]aultDuplicateHandlerImpl.java - liczba 6 jest magic number

True story, tutaj przydałby się co najmniej ogromny komentarz (a chyba nawet lepiej jeśli DuplicateHandler i algorytm skracający będą jakoś połączone, ta aby można było zobaczyć zależności).

https://github.com/Burdzi0/Js[...]llegalTimestampException.java
https://github.com/Burdzi0/Js[...]ervice/BlankUrlException.java - powinny rozszerzać RuntimeException (ewentualnie Exception), a nie Throwable

Do poprawy, ale mógłbyś rozwinąć?

https://github.com/Burdzi0/Js[...]ce/DefaultUrlServiceImpl.java - jest rzucany throw new ValidationException(); bez messega

Rzeczywiście, informacja się przyda.
Dzięki za pomoc! :)

0

@scibi92:

własnie, InMemoryRepository powinno używać ConcurrentHashMap zamiast HashMap.

Będzie poprawione.

I czemu w modelu massz settery zamiast robić model niemutowalny?

Jest mutowalny, bo obiekt zostaje stworzony, ale nie ma jeszcze wszystkich odpowiednich pól, tj. w najprostszej implementacji (UrlPojo.java) posiadam String url i OffsetDateTime expirationTime z których tworzony jest skrót. Chyba byłbym w stanie zmienić obiekt na niemutowalny, będzie wymagało to pewnych zmian, ale do zrobienia ;)

2
Burdzi0 napisał(a):

https://github.com/Burdzi0/Js[...]llegalTimestampException.java
https://github.com/Burdzi0/Js[...]ervice/BlankUrlException.java - powinny rozszerzać RuntimeException (ewentualnie Exception), a nie Throwable

Do poprawy, ale mógłbyś rozwinąć?

Po Throwable dziedziczy również (oprócz klasy Exception) klasa Error, która nie powinna być łapana. Dlatego customowe exceptiony powinny dziedziczyć po Exception, a najlepiej po RuntimeException, żeby nie musieć wszędzie try catcha pisać

3

Podpiąłem SonarQube i Codecov i właśnie te narzędzia pokazały mi, że np. konstruktory nie są pokryte testami. @Burdzi0 niewiele myśląc, chcąc być w przyszłości dobrym programistą, wziął się do pracy nie zastanawiając się czy jest tego sens Rzeczywiście trochę pofrunąłem (to samo z getterami i setterami). Czy takie części kodu wyklucza się z tego typu narzędzi? Nie zwraca uwagi, że pokrycie testami nie jest całkowite czy jak?

Taka sytuacja nie może wystąpić, chyba że brakuje ci jakichś innych testów. Przecież konstruktor takiego wyjątku zostanie wywołany w teście, który sprawdza czy taki wyjątek zostaje rzucony! Np. masz tam jakaś funkcje która robi walidacje parametrów i rzuca wyjątek jeśli cos jest nie tak. Pisząc testy dla tej funkcji piszesz testy na te specjalne sytuacje jakimś shouldThrowXExceptionForInvalidParameter i siłą rzeczy pokryjesz ten wyjątek.
Analogicznie z dowolną inną metodą -> skoro testy nie pokrywają jej wywołania to:

  • albo to jest jakiś entry-point i potrzebuje własnego testu, którego jeszcze nie masz
  • albo brakuje ci testu który sprawdza ścieżkę, gdzie ta metoda jest wołana
  • albo ta metoda nigdy nie jest wołana i należy się jej pozbyć

Taki protip dla getterów i setterów -> jeśli ich nie masz, to nie trzeba ich testować! :P

2

Podpiąłem SonarQube i Codecov i właśnie te narzędzia pokazały mi, że np. konstruktory nie są pokryte testami. @Burdzi0 niewiele myśląc, chcąc być w przyszłości dobrym programistą, wziął się do pracy nie zastanawiając się czy jest tego sens :P Rzeczywiście trochę pofrunąłem (to samo z getterami i setterami). Czy takie części kodu wyklucza się z tego typu narzędzi? Nie zwraca uwagi, że pokrycie testami nie jest całkowite czy jak?

To są narzędzia tylko. Zdrowy rozsądek jest zawsze najwazniejszy, a testowanie konstruktorów rozsądnie nie brzmi :P

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