Prośba o code review projektu webowego

Witam,

od października zainteresowałem się aplikacjami webowymi i zacząłem od JavyEE.
Potem jakoś od końca października zacząłem pisać proste CRUD'y w springu.

Pierwsza moja 'większa' aplikacja to było napisanie API dla bankowej aplikacji.
Druga napisana aplikacja to jest też aplikacja bankowa ale napisana w springu + thymeleaf. Po prostu wyciągnąłem wnioski bo napisaniu tamtego restowego API i chciałem to napisać lepiej i nauczyć się troche front-end'u bo słabo u mnie z tym, a przy tym i tak muszę napisać projekt na zaliczenie na studia przedmiotu. Prosiłbym żebyście zrobili code review na mojej aplikacji bo widzialem, że już takie tematy pojawiały się tu na forum :) W aplikacji mam zamiar napisać jeszcze testy najprawdopodobniej w spocku.
Link do githuba -> https://github.com/RobertKrzywina/Bank-Web-Application

Mam jeszcze pytanie co do mojego githuba. Przypiąłem sobie te 2 napisane aplikacje + repo z zadaniami algorytmicznymi ze spoja. Kod w tym repo jest z przed roku, początki mojego programowania i nie wiem czy usunąć to repo z przypietych czy zostawić jak jest. Co myślicie?

Jeszcze pytanie odnośnie nauki. Ogólnie to celuje w dostanie się na jakiś płatny staż na wakacje, dopiero marzec więc jeszcze czasu trochę mam. W thymeleafie raczej nie chce już nic robić, bo teraz tylko REST'owe aplikacje. Jest sens uczyć się javy + np. angulara? Będę startował na jakieś juniorskie stanowisko z javy więc myślę, że lepiej umieć dobrze jave, niż umiec średnio jave i troche angulara. Co myślicie? Czekam na konstruktywne opinie, pozdrawiam :)

1. gdzie są testy ja się pytam
2. adnotacja LoginPassword moglaby mieć w nazwie validation albo cokolwiek co by wskazywało na to co robi
3. ValidationFacade - czy warto z walidacji robić moduł? poza tym skoro masz moduł i fasadę i zakładam, że celem jest modularność to czemu wewnatrz modułu masz zależnosci do springa, a metody walidujace nic nie zwracają? gdyby były testy to i pewnie lepiej by to wyglądało, bo zaglądanie w bebechy binding result byłoby katorgą
4. bank_account wg specyfikacji błędna nazwa pakietu AFAIK
5. BankAccountRepository posiada metodę addAmountToReceiver - generalnie to klasa domenowa jest goła i wesoła - nie posiada logiki, natomiast logika, którą mogłaby posiadać jest wrzucona do repozytorium, powinno byc na odwrót
6. srednio sie orientuje w obsłudze transkacji przez springa, ale czy tutaj: https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/bank_account/BankAccountFacade.java#L52 nie masz osobnych transakcji na tych dwóch metodach? czyli jednemu zabierzesz, w drugim poleci wyjątek i powstała niespójność w systemie
7. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L39 - modyfikowanie argumentów jest słabe, lepszym rozwiązaniem byłoby zwrócic nowy obiekt, nawet tego samego typu niz grzebac w tym co metoda dostaje
8. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L44 namieszana domena z infrastrukturą, to całe wysyłanie maili mogłoby być wydzielone do oddzielnej klasy/modułu/whatever, srednio mi sie wydaje by była to odpowiedzialnośc modułu 'user'
9. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L110 magic number, czemu choice to int? czym jest ta magiczna 1?
10. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/User.java#L30 czy mi sie wydaje, ze trzymasz w bazie hasło tak jak przyszło i hasło zhashowane w osobnej kolumnie?
11. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L39 UserFacade i generateBankAccount - moze to powinien byc jeden moduł?

więc myślę, że lepiej umieć dobrze jave, niż umiec średnio jave i troche angulara.

zależy, jest sporo firm, w których jest podział na backend i frontend, ale obiektywnie łatwiej będzie ci coś znaleźc jak będziesz coś kumal z frontu

początki mojego programowania i nie wiem czy usunąć to repo z przypietych czy zostawić jak jes

nie patrzyłem, ale zakładam, że szału nie ma, lepiej jest mieć 1 repo, ale dobre

W aplikacji mam zamiar napisać jeszcze testy najprawdopodobniej w spocku.

następnym razem napisz testy i wtedy wrzuć do oceny. Generalnie jak na brak komercyjnego doświadczenia to myślę, ze jest nieźle, jednak mocno naciskam byś nauczył się pisać testy i je napisał zanim pójdziesz dalej

1. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/resources/application.yml#L25 dobre hasło! Ale tak serio, credentiale publicznie na githubie...
2. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/validation/LoginPasswordValidator.java#L33 tak zwana klasa z wajchą. Jak przesuniesz wajchę w prawo to robi X a jak w lewo to robi Y :D I następna https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/validation/IsUniqueValidator.java#L26
3. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L93 za dużo logiki w tej jednej biednej metodzie. Jakiś pętle, zagnieżdżenia. To należałoby rozbić.
4. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/UserController.java#L48 ładny copypaste z tym auth, a może by tak jednak zrobić jakiś ładny wrapper na to? :)
5. Tak generalnie to może warto byłoby to podzielić na moduły a nie tylko na pakiety? Wtedy ładnie wychodzi jak jest za dużo zależności między sobą.

1. gdzie są testy ja się pytam

beda

2. adnotacja LoginPassword moglaby mieć w nazwie validation albo cokolwiek co by wskazywało na to co robi

ok, poprawie

3. ValidationFacade - czy warto z walidacji robić moduł? poza tym skoro masz moduł i fasadę i zakładam, że celem jest modularność to czemu wewnatrz modułu masz zależnosci do springa, a metody walidujace nic nie zwracają? gdyby były testy to i pewnie lepiej by to wyglądało, bo zaglądanie w bebechy binding result byłoby katorgą

ok, poprawie

4. bank_account wg specyfikacji błędna nazwa pakietu AFAIK

bank.account incoming

5. BankAccountRepository posiada metodę addAmountToReceiver - generalnie to klasa domenowa jest goła i wesoła - nie posiada logiki, natomiast logika, którą mogłaby posiadać jest wrzucona do repozytorium, powinno byc na odwrót

fakt, rozumiem o co chodzi ale jak mam to zrobic bez repozytorium, przeciez musze wywolac to repo zeby cos zupdatowac?

6. srednio sie orientuje w obsłudze transkacji przez springa, ale czy tutaj: https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/bank_account/BankAccountFacade.java#L52 nie masz osobnych transakcji na tych dwóch metodach? czyli jednemu zabierzesz, w drugim poleci wyjątek i powstała niespójność w systemie

sluszna uwaga, to samo mialem z wysylaniem tokena na maila i powstala mi wlasnie taka niespojnosc w systemie

7. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L39 - modyfikowanie argumentów jest słabe, lepszym rozwiązaniem byłoby zwrócic nowy obiekt, nawet tego samego typu niz grzebac w tym co metoda dostaje

ok, poprawie

8. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L44 namieszana domena z infrastrukturą, to całe wysyłanie maili mogłoby być wydzielone do oddzielnej klasy/modułu/whatever, srednio mi sie wydaje by była to odpowiedzialnośc modułu 'user'

ok, pomysle nad tym i postaram sie cos z tym zrobic

9. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L110 magic number, czemu choice to int? czym jest ta magiczna 1?

faktycznie, gdybym nie ja pisal ten kod, nie wiedzialbym prawdopodnie co to robi

10. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/User.java#L30 czy mi sie wydaje, ze trzymasz w bazie hasło tak jak przyszło i hasło zhashowane w osobnej kolumnie?

tak, poprawilem juz to

11. https://github.com/RobertKrzywina/Bank-Web-Application/blob/master/src/main/java/pl/robert/project/user/domain/UserFacade.java#L39 UserFacade i generateBankAccount - moze to powinien byc jeden moduł?

pomysle nad tym

następnym razem napisz testy i wtedy wrzuć do oceny. Generalnie jak na brak komercyjnego doświadczenia to myślę, ze jest nieźle, jednak mocno naciskam byś nauczył się pisać testy i je napisał zanim pójdziesz dalej

Ok, jakis czas temu nie wiedzialem po co te testy sa, ale jak sie rozrasta projekt to nie chce sie wszystkiego przeklikiwac itp i dopiero teraz widze jak testy sa niezbedne. Musze sie nauczyc pisac je. Fajnie ze mimo tylu błedów napisales, ze jest niezle @hcubyc :) Bede poprawial na bieżąco, bo troche sie tego nazbieralo. Dzieki @Shalom oraz @hcubyc za code review, to naprawde duzo mi pomaga bo sam bym nie znalazl tyle bledow w kodzie

fakt, rozumiem o co chodzi ale jak mam to zrobic bez repozytorium, przeciez musze wywolac to repo zeby cos zupdatowac?

działaj na domenie, najlepiej na obiektach typu User/BankAccount, a jeżeli nie możesz to dopiero na jakis fasadach/serwisach.Czyli dla przykładu bankAccount.withdrawMoney(), bankAccount.transferMoney() i na samym koncu wolasz repository.save(bankAccount). i tam w srodku tych metod bank account sobie sprawdzasz wszystkie reguły, czy może, czy nie może etc w innym wypadku zamiast pisac kod 'domenowy', logikę biznesową bedziesz to pchał do SQLek