Ocena kodu Java back+front

Witam.

https://github.com/sovas1/ITcontractorsManager

Proszę o ocenę.
Jakby ktoś miał ochotę uruchomić to korzystałem z IntelliJ, Mavena i pluginów Lombok oraz AngularJS.

Projekt chciałbym umieścić w cv, wiem że jest dość ubogi, ale uczę się Javy dopiero 3 miesiące i nie mam nic lepszego a puste CV słabo wygląda.

Wszelka krytyka bardzo mile widziana.

PS: Co jakiś czas wrzucam commita jak mam chwile czasu popracować, do poprawy na pewno jest front (to był mój pierwszy kontakt z frontem), i dojdzie kilka opcji i usprawnień, najpierw chce uporządkować to co jest.

Dzięki!

Przede wszytkim, jak dla mnie super robota.

Moje spostrzeżenia:

1. Funkcjonalność, to się zupełnie nie trzyma kupy. Dodaj kontraktora do firmy przez wybranie id. No słabe :)
2. Te powiadomienia jako dialog boxy, czy jak to tam się zwie, są słabe. Zrobiłbym normalną walidację springową. Kwestia gustu.
3. Brak testów, przecież spring udostępnia takie fajne testy integracyjne, całe api sobie można przetestować.
4. Nie ma serwisów, logika w kontrolerze, to chyba takie czepianie się na siłę, ale napiszę, niech mi tam.
5. I główna moja zagwozdka, po co tak podzieliłeś projekt na /repository i /dao? Coś tu nie gra IDE mi podkreśla na czerwono sporo rzeczy.

Pozdro i gratki.

EDIT:

Ad4. Niepoprawnie zinterpretowałem tę logikę w kontrolerze. Jest ok. Wszystko przez to zamieszanie z repository i dao. Jestem zupełnie inaczej "nauczony".
Ad5. Faktycznie nie miałem wtyczki do lomboka.

Przejrzałem pobieżnie i jest w porządku jak na 3 miesiące nauki. Niemniej jednak jakość kodu front-endu jest słaba. Przerzuć logikę biznesową z kontrolerów do serwisów. Każdy serwis, kontroler, dyrektywa etc. powinny znajdować się w osobnym pliku. Tyle ze zmian koniecznych, poza tym polecam zapoznać się z https://github.com/johnpapa/angular-styleguide/blob/master/a1/README.md Generalnie im lepszy kod tym lepsze sprawisz wrażenie.

wy tak poważnie? Ja się uczę Javy od 4 miesięcy i jestem na etapie robienia "dzienników szkolnych". A wy po 3 miesiącach walicie takie projekty, że nawet nie wiem gdzie wejść żeby sprawdzić kod. Mój Boże:D

W zasadzie to Ja sam nie wiem czego Ja się w końcu cholera uczę. Mam mega zajawkę na programowanie. To znaczy... W zasadzie na Jave bo jakoś inne języki mnie do siebie tak nie ciągną. Na początku kiedy poznawałem podstawy z jakiś kursów czułem się taki wyśmienity w tym co robiłem. Pojmowałem wszystko. No ale kurs się skończył, Ja zostałem sam i zaczął się martwy punkt. Bo nie wiedziałem co ze sobą zrobić. I tak sobie klęczę tutaj i klęczę. Teraz czuję się bezsilny i żałosny widząc co inni potrafią przez ten czas:D

uwagi do kodu AngularJS:
https://github.com/sovas1/ITcontractorsManager/blob/master/src/main/resources/static/app/script.js
dobrą praktyką jest owijanie całego pliku w samo wywołującą się funkcję, bo inaczej zmienna var urlBase będzie globalna i może skonfliktować się potem z innymi plikami.

// samowywołująca się funkcja
(function () {
 // reszta pliku
)()

To co napisałem nie byłoby potrzebne, jeślibyś stosował jakiś system modułów (np. CommonJS, import/export z ES6) i byś to sobie budował jakoś(Browserify, Babel, Webpack itp.), wtedy masz izolację poszczególnych skryptów z automatu. Ale z tego co widzę to takiego builda nie stosujesz, tylko przypinasz bezpośrednio skrypt, więc to co jest w nim deklarowane jest globalne.

Druga uwaga, że okej, dodałeś Angulara, zrobiłeś route'y do podstron, zrobiłeś kontrolery, wszystko ładnie, z tym wyjątkiem, że w ogóle nie używasz dyrektyw, które są jednym z najbardziej pożytecznych elementów Angulara. Dyrektywy pozwalają ci zdefiniować reużywalne komponenty, które łączą szablon w HTML z logiką, która na nim operuje (dyrektywy mogą posiadać własne kontrolery czy funkcję link do inicjalizacji) i własnym scope (jeśli masz taką potrzebę). Czyli wszystko staje się bardziej obiektowe, jak klocki lego.

Fajnie jakbyś zrobił demo on-line, nie każdy ma możliwość odpalenia tego lokalnie

I w readme daj uruchomienie raczej $mvn spring-boot:run, skoro dodałeś ten plugin do pom.xml to czemu z niego nie korzystać? Nie każdy korzysta z intellij idea, nie ma co robić reklamy i nie każdemu będzie się chciało odpalać projekt w IDE. A mvn prawie każdy javowiec posiada.

Poprawiłem layout, jest trochę przyjaźniejszy w odbiorze i bardziej responsywny.
Jutro postaram się zrobić testy API w mockito i poprawić skrypt angulara zgodnie z zaleceniem @LukeJL.

Wołam @Shalom jakbyś miał czas to zerknij co jest do poprawy. Dzięki!

Może logowanie dodaj z wykorzystaniem AoP jeśli chcesz się czegoś nowego nauczyć.

@mariusz_s rzuciłem okiem tylko że... tam nic nie ma ;] W sensie ten projekt to jest taki Generic CRUD application, to przeklikania z tutoriala w pół godziny. Nie ma w nim tak na oko błędów, ale też trudno jakieś narobić jak program nic nie robi :D

Ogólnie jest ok. Usuąłbym zakomentowane zależności w pomie, żeby nie zaśmiecać projektu. Jeżeli gdzieś jeszcze masz zakomentowany kod, to go też warto usunąć. Widziałem w niektórych miejscach null-checki na jakichś danych z modelu. Jeśli piszesz w Javie 8 lub używasz Guavy, to warto skorzystać z Optionala. W jednym miejscu masz komentarz "removing contractor from list in company". Staraj się unikać takich komentarzy. Spróbuj wydzielić komentowany kod do metody o nazwie np. "removeContractorFromCompany()" lub innej odpowiedniej. Formatowanie kodu jest w niektórych miejscach niekonsekwentne i niespójne. Możesz zapiąć do projektu CheckStyle i spójnie sformatować kod we wszystkich miejscach. Możesz też dorzucić statyczną analizę kodu (np. PMD, FindBugs, etc.). Ponadto testy są trochę ubogie. Spróbuj dodać ich więcej. Ponadto w tych testach, które masz, możesz wsadzić kontraktorów do jakiejś listy i pomyśleć nad parametryzacją tego testu (zerknij chociażby na ten projekt: https://github.com/Pragmatists/JUnitParams). Widziałem, że masz tam Lomboka. Dawno go nie używałem, ale o ile pamiętam, generuje on metody toString(), a Ty i tak je niepotrzebnie dopisywałeś w klasach z adnotacjami lombokowymi. Warto zbadać temat. Możesz też postarać się, aby kod był jak najbardziej niemutowalny, aby nie można było niepotrzebnie modyfikować obiektów po ich utworzeniu, co czasami powoduje błędy w działaniu aplikacji. Z nowościami we front-endzie nie jestem zbytnio na bieżąco, więc tutaj nie będę doradzał. W każdym razie, widzę, że jest bower do zależności, więc to jest na plus.

Taka drobna rada - poskracaj sobie warunki w js bo są przerażająco długie, możesz to co masz zrefaktoryzować chociażby w ten sposób:

$scope.companyName == null || $scope.companyName == "" => !!!$scope.companyName

$scope.companyId < 1 || $scope.companyId == null => +$scope.companyId < 1

itd

Dziś wolny wieczór więc dodałem kilka poprawek, o których wspomnieliście:

• podzieliłem script.js na pliki i wstrzyknąłem zależności,
• dodałem custom directives, żeby nie zaśmiecać indexu
• wywaliłem CDN, wszystko jest teraz offline
• pom.xml wyczyszczony ze zbędnych komentarzy
• dodałem komentarze do toString()
• obiekty w teście tworzone są teraz na liście

@Lectre jakbyś miał chwilę to zerknij na ten mój nieszczęsny front proszę ;)

Nadal nie wydzieliłeś kontrolerów do osobnych plików. Jeden plik, jeden komponent.

Źle:

var app = angular.module('models-controller', []);
app.controller (...)

Dobrze:

angular.module('app.something', []); // Myśl o modułach jak o Javowych pakietach
angular.module('app.something').controller (...)

To wygląda strasznie. Wyrzucaj do osobnej funkcji, łatwiej debugować.

 if ($scope.contractorName == "" || $scope.contractorName == null ||
      $scope.technology == "" || $scope.technology == null ||
      $scope.yearsOfExperience < 0 || $scope.monthlySalary < 0 ||
      $scope.yearsOfExperience == null || $scope.monthlySalary == null) {
      alert("Insufficient Data! Please provide values for Contractor.");
      }

Nadal brak serwisów. Wszystko siedzi w kontrolerach, brak uporządkowania kodu. Nie przeczytałeś style guide, który ci podrzuciłem. Ulepszyć można tutaj bardzo dużo, ale nie wszystko warto :P

Generalnie do front-endowca mi daleko, ale jestem w projekcie gdzie też mamy Angulara i trochę tym przesiąkłem.

Dobry kontroler ma w sobie model i funkcje wywoływane w widoku (np. ng-click). Brak manipulacji DOMem. Brak logiki biznesowej.

Kontroler ma kontrolować. Być łącznikiem. Wyobraź sobie, że jesteś kontrolerem w realnym świecie. Twój kolega (widok) dzwoni i mówi, że znalazł na polu rdzewiejący, metalowy obiekt w kształcie kręgla. Ale ty (kontroler) jesteś łebski i nie zajmujesz się tym sam, tylko dzwonisz po speców (serwis). Taki to już z ciebie spryciarz.

Przejrzyj sobie to, nie musisz wszystkiego już teraz rozumieć. https://demisx.github.io/angularjs/2014/09/14/angular-what-goes-where.html

Kolejny update

• wydzieliłem logikę z kontrolerów do serwisów
• usunąłem przydługawe warunki i zamieniłem je na walidacje na poziomie html

@Lectre