Wątek przeniesiony 2019-03-01 10:48 z przez Ktos.

Prośba o code review projektu webowego

Odpowiedz Nowy wątek
2019-02-28 17:06

Rejestracja: 1 rok temu

Ostatnio: 1 rok temu

0

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

edytowany 1x, ostatnio: AnonymousProgrammer, 2019-02-28 17:07

Pozostało 580 znaków

2019-02-28 23:03

Rejestracja: 7 lat temu

Ostatnio: 2 tygodnie temu

0
  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/RobertKrzy[...]nt/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/RobertKrzy[...]er/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/RobertKrzy[...]er/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/RobertKrzy[...]r/domain/UserFacade.java#L110 magic number, czemu choice to int? czym jest ta magiczna 1?
  10. https://github.com/RobertKrzy[...]ect/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/RobertKrzy[...]er/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


Limitations are limitless

> ##### Ola Nordmann napisał(a)
> Moim językiem ojczystym jest C++ i proszę uszanować to, że piszę po polsku.
edytowany 3x, ostatnio: hcubyc, 2019-02-28 23:07

Pozostało 580 znaków

2019-02-28 23:18
Moderator

Rejestracja: 16 lat temu

Ostatnio: 1 godzina temu

1
  1. https://github.com/RobertKrzy[...]resources/application.yml#L25 dobre hasło! Ale tak serio, credentiale publicznie na githubie...
  2. https://github.com/RobertKrzy[...]ginPasswordValidator.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/RobertKrzy[...]on/IsUniqueValidator.java#L26
  3. https://github.com/RobertKrzy[...]er/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/RobertKrzy[...]/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ą.

Masz problem? Pisz na forum, nie do mnie. Nie masz problemów? Kup komputer...
@Shalom: do 1: A może chciał abyś się zalogował i miałby twojego ipka? :) - WeiXiao 2019-02-28 23:37
1. Nie wiem jak to zahashować w configu, poszukam 2. Ok z tym pierwszym linkiem zgadzam sie, jest do bani :D ale z tym drugim to dlaczego jest kod zly? Dla mnie jest klarownie napisane co ta metoda robi, nawet jakbym tego kodu nie pisal 3. Postaram sie to jakos ladniej napisac bo jest troche syf 4. Tak, myslalem nad tym dzisiaj zeby to poprawic ale cos kod nie dzialal, poprawie to 5. Pierwsze slysze o modulach, poczytam :) - AnonymousProgrammer 2019-03-01 00:29
Ad 1 to takie rzeczy albo się wrzuca w runtime zmiennymi środowiskowymi, albo szyfruje a klucz do deszyfrowania wrzuca w runtime. Tak żeby jednak w repo nie wisiały gołe credentiale ;) - Shalom 2019-03-01 00:32

Pozostało 580 znaków

2019-03-01 01:11

Rejestracja: 1 rok temu

Ostatnio: 1 rok temu

0
  1. gdzie są testy ja się pytam

beda

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

ok, poprawie

  1. 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

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

bank.account incoming

  1. 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?

  1. srednio sie orientuje w obsłudze transkacji przez springa, ale czy tutaj: https://github.com/RobertKrzy[...]nt/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

  1. https://github.com/RobertKrzy[...]er/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

  1. https://github.com/RobertKrzy[...]er/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

  1. https://github.com/RobertKrzy[...]r/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

  1. https://github.com/RobertKrzy[...]ect/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

  1. https://github.com/RobertKrzy[...]er/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

edytowany 1x, ostatnio: AnonymousProgrammer, 2019-03-01 01:11

Pozostało 580 znaków

2019-03-01 08:12

Rejestracja: 7 lat temu

Ostatnio: 2 tygodnie temu

0

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


Limitations are limitless

> ##### Ola Nordmann napisał(a)
> Moim językiem ojczystym jest C++ i proszę uszanować to, że piszę po polsku.
edytowany 1x, ostatnio: hcubyc, 2019-03-01 08:12

Pozostało 580 znaków

Odpowiedz

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