Code review aplikacji REST-owej w Spring Boocie.

Odpowiedz Nowy wątek
2018-12-18 23:27
0

Jakiś czas temu zabrałem się za refaktoryzację swojej starej aplikacji, napisanej w Spring-Boocie. Tematyka, to Symulator giełdy Kryptowalut.. Tutaj wątek w którym prosiłem o ocenę starego kodu: Code review. .Teraz już powoli kończę wcześniej wspomnianą refaktoryzację. Postarałem się zmienić pakietowanie z per warstwa na per funkcjonalność. Powywalałem zewsząd settery (w końcu zrozumiałem na czym polega immutability). Starałem się pisać z jak najmniejszą ilością klas publicznych. Przeważnie jedna klasa Facade publiczna w pakiecie, nie licząc dtosów oraz Enumów (klasę Cryptocurrency traktuję jako Enum mimo, że jest to klasa zapisywana w bazie danych). Zmieniłem przechowywanie informacji w sesji, na rzecz tokena - oglądnąłem materiał nat. GC i imo - podejście bardziej przyjazne dla GC oraz mniej pamięciożerne na serwerze. Muszę jeszcze zmienić mechanikę wyciągania OwnedCryptocurrency (wyciągam z klasy User, zamiast z bazy danych po User ID). W bazie danych mam dwa triggery odpowiedzialne za zmianę statusu oferty i dodanie użytkownikowi usd/owned_cryptocurrency.

Github: https://github.com/KamLar/mar[...]amil/larysz/marketsimulatorv2

Jeżeli mógłbym prosić, to prosiłbym o wytknięcie błędów które jeszcze mam. Zastanawiam się też nad trzema rzeczami:

  1. Typy proste vs Obiekty. Czy nie lepiej korzystać z typów prostych zamiast z obiektów tam gdzie to możliwe? Czy w klasach @Entity lepiej ze względów bezpieczeństwa korzystać z BigDecimal zamiast np double? Po pierwsze mniej pamięci zżerają, a po drugie są mniej obciążające dla GC.

  2. Lepiej, gdy klasa @Entity dostarcza metodę zwracającą DTO, czy lepiej zostawić to dla jakiegoś Mappera? Wiem, że i jedno i drugie ma swoje korzyści i prawdopodobnie zależy to od założenia (np Mapper zapewnia więcej możliwości zmapowania Obiektu, natomiast metoda w klasie jest imo bardziej intuicyjna).

  3. Bardziej porada niż pytanie. Każdy wie, że od juniora java dev, wymaga się nie tylko javy, springa, wzorców projektowych, clean-code, ale także dobrej znajomości SQL, Angulara, AWS itd. Czy na chwilę obecną, po code-review, mógłby ktoś stwierdzić na czym powinienem się teraz skupić? Douczyć się Springa, SQL, czy może czegoś innego (np. WebFluxa)? Subiektywnie mogę stwierdzić, że Javę w miarę umiem, Springa znam podstawy, SQL - nie wiem jak to przetłumaczyć ale very-basic. Wzorce projektowe i clean-code - też mi się wydaje, że znam (bardziej teoretycznie niż praktycznie, np. prostsze programy jestem wstanie komuś zrecenzować i wytknąć błędy, albo czasami wiem, że w danej chwili "rzeźbię w brązie" ale na chwilę obecną nie umiem zaimplementować tego tak jakbym chciał od początku).

Każdemu dziękuję za informację zwrotną.

edytowany 3x, ostatnio: furious programming, 2018-12-19 17:09
Popraw tytuł wątku i napisz w nim czego dotyczy ten code review. Ten dział służy wyłącznie do tego, więc samo code review w tytule nic nie wyjaśnia. - furious programming 2018-12-19 00:03
Ok. Już poprawione. Myślałem, że tagi są na tyle dobrze dobrane że jasno wskazują czego dotyczy ten code-review i nie trzeba w tytule tego dodawać. - Aisekai 2018-12-19 00:10
Najlepiej podać w tytule po prostu nazwę aplikacji/algorytmu i kilka słów więcej. Chyba że apka nie ma swojej nazwy, to przynajmniej kilka ogólnych szczegółów. - furious programming 2018-12-19 00:11

Pozostało 580 znaków

2018-12-19 16:04
0
  1. Brak testów
  2. Pomieszana koncepcja java based i annotation based configuration
  3. Użycie Either bez typów, nie widać co jest w środku
  4. Implementacja enum ErrorCode, lepiej dadać konstruktor z patametrem message i getMessage zamiast nadpisywać toString w każym objekcie.
  5. Można użyć Lombok aby pozbyc sie niepotrzebnego kodu(getters, setters, hashcode, equals...)

Pozostało 580 znaków

2018-12-19 16:30
0

Z takich rzeczy, które rzuciły mi się w oczy tak po szybkim przeglądnięciu struktury:

  • masz drobne błędy w nazewnictwie, np [...]Encodder.java, zawsze warto zwrócić na takie szczegóły uwagę ;)
  • rozumiem, że wolisz użyć strukturę podzieloną wg. funkcjonalności, niż wg warstwy, ale w chwili obecnej masz tam trochę wymieszanie, np. w [...]/domain masz trochę klas związanych z użytkownikiem, trochę z kryptowalutą, a trochę z enkodowamiem/dekodowaniem haseł - moim zdaniem tu powinny się jeszcze pojawić jakieś podfoldery, domain jest troszkę zbyt obszerne
  • hasła powinny być hashowane jakąś mocną (i powolną, np jakiś Blowfish czy PBKDF2), tak by nie można było ich dekodować ;) samo enkodowanie jest tak samo bezpieczne, jak trzymanie hasła w plaintext ;)
  • mam nadzieję, że prawdziwych haseł do bazy nie commitujesz, tylko trzymasz w lokalnej konfiguracji? ;)

@p_maciek: mi się już jakiś taki brzydki zwyczaj z Kotlina wyrobił, że jak mam jakąś data class (czy to encję, czy DTO) to pola są publiczne i niemutowalne i tyle, bez getterowej biegunki czy używania Lomboka ;)


Prosząc o pomoc w wiadomości prywatnej odbierasz sobie szansę na otrzymanie pomocy od kogoś bardziej kompetentnego :)
edytowany 1x, ostatnio: superdurszlak, 2018-12-19 16:31

Pozostało 580 znaków

2018-12-19 17:13
0

Dzięki za sprawdzenie.
p_maciek:

  1. Tak, testów mam mało ale raczej starałem się testować pod kontem tego, czy dana funkcjonalność działa, a nie czy np. OrderMapper dobrze Mapuje. Które podejście jest lepsze?
    2, 3, 4. Okej, już zmieniam. Fakt, masz rację.
  2. Lomboka jakoś nie lubię, a także słyszałem dużo negatywnych opinii. Niekorzystanie z Lomboka imo daje więcej korzyści niż korzystanie.

superdurszlak:

  1. Ok, już zmieniam nazwy.
  2. Na początku bardziej lubiłem strukturę per warstwa. Łatwiej o "poprawne pakietowanie". Od niedawna przeszedłem na per funkcjonalność i package-scope. Przy czym, zauważyłem że mam syf w pakietach. Chciałem zastąpić to modułami, ale wtedy wszystkie moduły musiałbym importować (nawet moduły Springowe) co mi się nie podobało, a nawet powiem więcej nie wychodziło mi to. Teraz chyba wiem jak to naprawić i postaram się to zmienić.
  3. Źle nazwałem interfejsy. Nie jest to szyfrowanie, tylko hashowanie. Hasła mam shashowane za pomocą BCrypt, natomiast Token ... tokena to randomowy ciąg znaków w miarę długi. W bazie przechowuję tą wartość (jako String) i wyszukuje Login i shashowane hasło, potem staram się wyszukać Użytkownika o podanym Loginem i Hasłem (z BCrypt.checkpw()).
  4. W DTO w sumie fakt, mógłbym tak zrobić. Zmienić wszystkie pola z private na public final. Ale w encjach? W sumie, też by tak można było zrobić i wtedy każda zmiana obiektu to utworzenie nowego z zmienionymi polami i go zapisanie do Bazy Danych (albo od razu SQL zmieniający stan obiektu).
  5. No niestety commituje, wiem że jest to błąd i powinienem to trzymać w lokalnej konfiguracji. Ale wychodzę z założenia, że do czasu aż jest to tylko projekt do jakiegoś CV (czy żeby się czegoś nauczyć) to mogę takie dane nawet udostępnić (w bazie danych na razie nie mam żadnych wrażliwych danych, a sama baza jest przechowywana lokalnie).
    Pisząc tą wypowiedź zrozumiałem dodatkowy sens interfejsów (można zobaczyć u mnie w projekcie, że jest ich niewiele a tak naprawdę mogłoby ich nie być).

Pozostało 580 znaków

2018-12-19 18:10
1
Aisekai napisał(a):

Dzięki za sprawdzenie.
p_maciek:

  1. Tak, testów mam mało ale raczej starałem się testować pod kontem tego, czy dana funkcjonalność działa, a nie czy np. OrderMapper dobrze Mapuje. Które podejście jest lepsze?

A czemu nie oba? ;)

  • testujesz funkcjonalności by mieć pewność, że działają, są idiotoodporne (w sensie takim, że zamiast crasha czy wys**nia defaultowego HTML 500 w REST API będzie jakiś spójny JSON response, na przykład), czy dozwolone akcje robią, co mają robić, czy niedozwolone są blokowane itd. Plus jest też taki, że nie musisz się przeklikiwać przez UI by sprawdzić, czy działa i głowić się, czy błąd jest na froncie czy w backendzie, albo męczyć żywy organizm Postmanem - odpalasz gradlew test czy coś w ten deseń i przelatujesz po API.
  • testujesz jednostkowo pojedyncze klocki, które są za coś odpowiedzialne - jeśli jakiś mapper bierzesz z frameworka, średnio jest sens testować framework (albo robić słynne testowanie Mockito), ale jeśli implementujesz go na piechotę, warto sprawdzić, czy poprawnie mapuje - choćby dlatego, żeby uniknąć regresji w przyszłości, nawet jeśli teraz jesteś pewien, że działa dobrze. Jak dokonujesz na piechotę jakiejś konwersji np. płaskiej kolekcji w jakąś inną strukturę danych, warto napisać UT które będą sprawdzać poprawność tych konwersji i tak dalej ;)
  1. Lomboka jakoś nie lubię, a także słyszałem dużo negatywnych opinii. Niekorzystanie z Lomboka imo daje więcej korzyści niż korzystanie.

Zawsze możesz podpuścić @jarekr000000 by się wypowiedział o Lomboku ;) tymczasem podrzucam drugi wynik wyszukiwania frazy "Jarek Ratajski Lombok" ;)
screenshot-20181219175745.png

  1. Źle nazwałem interfejsy. Nie jest to szyfrowanie, tylko hashowanie. Hasła mam shashowane za pomocą BCrypt, natomiast Token ... tokena to randomowy ciąg znaków w miarę długi. W bazie przechowuję tą wartość (jako String) i wyszukuje Login i shashowane hasło, potem staram się wyszukać Użytkownika o podanym Loginem i Hasłem (z BCrypt.checkpw()).

O, nazywanie klas i metod tym, czym są i co robią też jest ważne, choć i tak wolę już PasswordEncoder który hashuje niż matematyków żonglujących w Pythonie funkcjami w stylu:

def avgsm_gdfgrph_wghts_opt_m_4(wghts, tutututu, wolololo, xyz, abc, g, a, s, m, ping, pong, mao, tse, tung):
  1. No niestety commituje, wiem że jest to błąd i powinienem to trzymać w lokalnej konfiguracji. Ale wychodzę z założenia, że do czasu aż jest to tylko projekt do jakiegoś CV (czy żeby się czegoś nauczyć) to mogę takie dane nawet udostępnić (w bazie danych na razie nie mam żadnych wrażliwych danych, a sama baza jest przechowywana lokalnie).

Ale wiesz, zawsze to zła praktyka o którą ktoś kiedyś może się czepić, poza tym im więcej tego typu kwiatków machniesz ucząc się, tym większe ryzyko, że wejdzie Ci w krew albo zwyczajnie zapomnisz któryś z tych kwiatków wyrwać przed potencjalnym releasem i co wtedy :o


Prosząc o pomoc w wiadomości prywatnej odbierasz sobie szansę na otrzymanie pomocy od kogoś bardziej kompetentnego :)
edytowany 1x, ostatnio: superdurszlak, 2018-12-19 18:11

Pozostało 580 znaków

2018-12-19 23:14
0

Okej, a jeszcze jedna ważna rzecz. Jeżeli tworzę sobie pakiety, to jedyne dostępne "Klasy" to powinny być interfejsy (żeby można utworzyć obiekt typu interfejsowego) + klasa konfiguracyjna z której można pobrać dany obiekt? Czy udostępniając fasadę, lepiej udostępnić sam interfejs fasady czy klasę? Jeżeli konfiguruję za pomocą klas (nie xml, nie adnotacje) to czy Controlery (albo RestControlery) też powinienem konfigurować za pomocą klas? Czy klasy konfiguracyjne można nazwać Fabrykami? W sensie z konfiguracji mogę pobrać (lub utworzyć) jakiś obiekt, czyli w jakiś sposób jest to podobne do AbstractFactory, czy tak?

Pozostało 580 znaków

2019-01-02 16:07
0

W mojej ocenie jest to tak:

  1. Jedynymi dostępnymi klasami powinny być fasady(czy to przez klasę czy interferjs)
  2. Wszystko konfigurujemy w ten sam sposób, czy to przez klasy, czy adnotacje.
  3. Klasy konfiguracyjne to konfiguracja aplikacji(inaczej fabryka beanow springowych). Raczej nie jest to fabryka z punktu widzenia wzorców obiektowych, gdzie mamy do czynienia z hierarchią super i sub klas.

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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