Prośba o code review

0

Witam

Chciałbym prosić o ocenę moich projektów. Będę naprawdę wdzięczny za każdą radę, wskazówkę oraz inne informacje, które zmienią mój świat na lepsze.

Github: https://github.com/mmazur658
Strona: https://marcinmazur-portfolio.pl/

Z góry dziękuję za poświęcony czas.
Pozdrawiam

0

Ja bym ten web calculator wywalił z portfolio, bo tylko działa na szkodę. I szkoda też że jak wybieram element z sidebara na telefonie to ten sidebar się potem nie zamyka.

0

Jakieś licencje?

0
lion137 napisał(a):

Jakieś licencje?

Mogę prosić o rozwinięcie pytania?

0

Potrzebujesz interface jesli masz tylko jedną implementacje? ;)

1

Masz te aplikacje gdzieś publicznie zdeployowane?

EDIT

Podajesz ze projekty mavenowe, ja nie mogłem znaleźć pom.xml i nie jestem w stanie uruchomić projektu/testów lokalnie.

Pierwsza rzecz której nie lubię to podział paczek na controller, service itp i pakowanie tam controllerów z całej aplikacji. Jak się projekt rozrasta to mamy X klas w takiej paczce i znalezienie controllera bez pamiętania nazwy to koszmar. Pomyśl o paczkach tematycznie, np. user może mieć pod sobą tylko controllery i servisy związane z kontem użytkownika. Takie paczki łatwiej też wyodrębnić do użycia w innych projektach. Poszukaj na yt wykładu Jakuba Nabrdalika na ten temat.

JSP... serio? Chcesz w tym pracować? Czy na takie tutoriale trafiłeś? Thymeleaf niby lepiej hmm. Ogólnie widać, że trochę czasu zaangażowałeś w te projekty i w tych technologiach powinieneś pracę znaleźć bez problemu.
Jeśli jednak chcesz dalej angażować czas w swój rozwój to proponuję przesiadkę na Single Page Application. Pod względem pracy pewnie Angular albo React (wolałbym Reacta), dla własnych projektów lub na start polecam Vue. A więc SPA na front, REST backend.
Osobiście nie lubię hibernate i jak patrzę na te sqle w stringu (u Ciebie jakieś hql i częściowe zapytania w stringach) to mi niedobrze :) Sam korzystam ze Spring Data, bez hibernate, działam na kolekcjach javowych i logikę piszę w javie niezależnie od frameworka. Póki co mi się podoba, może też tak spróbujesz.

0

Dziękuję wszystkim za poświęcony czas oraz za wasze cenne wskazówki i porady. W szczególności dziękuję @kkojot. Twoja odpowiedz jest bardzo treściwa i wyznacza mi nowy, interesujący kierunek rozwoju.

1

No to jedziemy :) nie napisałes, które repo, wiec wziąłem Simple-CRUD-Application

  1. wrzuce .settings do repo, to sa ustawienia eclipsa, pozwol innym miec wlasne (lub na inne IDE)
  2. commity po polsku, tak srednio to wyglada
  3. wrzucone jakies jary na gita, po co to komu, przeciez masz mavena
  4. wrzucony folder target do gita
  5. gdzie są testy
  6. metody na ponad 100 linii, polecam przeczytac clean code
  7. nazwy zmiennych/pol nie zgodne z konwencja np. gbc_salaryTextField
  8. gdzie są klasy z logiką? nie widze, wiec zakladam, ze logika siedzi w UI czyli w tych wszystkich klasach *Dialog. Nie jest to najlepszy design jaki można wykombinowac, fajnie gdyby logika była wydzielona do osobnych klas i działała na abstrakcyjnych klasach (chodzi o byt abstrakcyjny, a nie ze klasa ma miec slowko kluczowe abstract) tak zeby nie zalezala od UI/infrastruktury/jakiejkolwiek technologii czy bibliotek.
2

No dobra to patrząc po commitach biere web-calculator:

  1. WebCalculatorApplicationTests -> pusty test
  2. https://github.com/mmazur658/web-calculator/blob/master/src/test/java/pl/mazurmarcin/webcalculator/utils/CalculatorStatUtilsTest.java -> formatowanie popraw, czasem po kilka wolnych linii, które nie wiadomo czemu słuzą, bardzo duże metody testowe, można by ładnie to podzielić zazwyczaj tam gdzie masz pare linii kodu oddzielonych enterami to z tego mógłbyś wyciągnąc osobną metodę, testujesz CalculatorStatUtilsImpl, czyli implementacje - a gdzie testy zachowania? Czy CalculatorStatUtilsImpl to jedyna implementacja danego interfejsu? Jeżeli tak to bez sensu.
  3. shouldGetToday co ten test robi? Sprawdza czy twoja klasa zwróciła dokładnie to samo co SDF?
  4. ServiceUtilsImpl abstrahujac od suffixu Impl w nazwie, ktory zaciemnia bo nic nie wnosi - czy idzie dac lepsza nazwę niz ServiceUtils?
  5. testujesz klasy w bardzo mocnej izolacji, myslales nad jakims testem, ktory by sprawdzil chociaz główną scieżke? Czy to wszystko ze sobą gada?
  6. https://github.com/mmazur658/web-calculator/blob/master/src/main/java/pl/mazurmarcin/webcalculator/services/ContactFormMessageService.java#L39 w takiej metodzie komentarz wydaje sie byc zbedny, nazwa metody powinna jasno mowic co zostanie zwrócone
  7. tak samo jak ten komentarz: https://github.com/mmazur658/web-calculator/blob/master/src/main/java/pl/mazurmarcin/webcalculator/services/CalculatorStatServiceImpl.java#L30
  8. https://github.com/mmazur658/web-calculator/blob/master/src/main/java/pl/mazurmarcin/webcalculator/utils/SearchEngineUtilsImpl.java#L25 bardzo długa metoda, przydałby się refactor. Tak samo możnaby wykorzystac Stream API wprowadzony w Javie 8
  9. https://github.com/mmazur658/web-calculator/blob/master/src/main/java/pl/mazurmarcin/webcalculator/utils/ServiceUtilsImpl.java#L23 tworzysz klasę przez ustawienie pól setterami. Czy te settery są ci w ogóle potrzebne? Czy nie lepiej byłoby mieć niemutowalny obiekt i tworzyć go konstruktorem?
  10. https://github.com/mmazur658/web-calculator/blob/master/src/main/java/pl/mazurmarcin/webcalculator/utils/ServiceUtilsImpl.java#L69 wygląda jak anemic entity. Czy nie byloby lepiej gdyby klasa ContactFormMessage posiadała te logikę zamiast wywalać jej bebechy na wierzch? Law of demeter
  11. klasa ServiceUtilsImpl tworzy rózne obiekty, zmienia im stan, tworzy zapytania HQLowe... troche dużo odpowiedzialnosci, a kodu mało, więc z biegiem czasu może się rozrosnąc do God Object
0
> grep -c -h '\S' **.java | datamash count 1 sum 1
54      5111
> find -printf '%d\n' | datamash max 1
9
> 

54 pliki *.java zawierające 5111 niepustych linii. Wysokość drzewa 9. Nieźle jak na kalkulator.

UNIX V1 (kernel + Thompson Shell):

> grep -c -h '\S' **.s | datamash count 1 sum 1
13      4501
> find -printf '%d\n' | datamash max 1
1
> 

:D

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