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! :)