Optional'e, wyjątki, ocena projektu

Z całym szacunkiem nie wygląda to najlepiej :D Dobra, no to lecim z tym koksem:
1)Zamiast dzielić projekt "pionowo" podzieliłbym poziomo, czyli pakiety repository, entity, controller etc.
2)Po co kwiatki w stylu UserPersistence ? Właściwie to widze że to głównie wywołanie operacji z Repository, ale jest na przykład

private String getLoggedUser(){
        return SecurityContextHolder.getContext().getAuthentication().getName();
    }

To później przekazywane jest do innej metody która ma zmienić hasło. To jest architektoniczna tragedia, jeśli mamy repository to na pewno nie powinno wiedziec jaki jest użytkownik zalogowany.
Do metody ze zmianą hasła powinno być przekazane hasło i login. To chyba jednak ma byc obiekt od logiki? W takim razie nazwa niezbyt trafna. Zreszta własnie zauważyłem że nazwałeś jest z adnotacją @Service. Słabo, zmienić nazwe na UserService
3)Operowanie na encjach bezpośrednio, nie korzystanie z DTO -> słabo. Poczytaj o transakcjach w JPA

@scibi92 nie zgadzam się z pomysłem o twoim podziale. Nie ma nic złego w tym co OP zrobił. Ba, często ma to dużo więcej sensu jeśli te moduły są na tyle niezależne że można z nich zrobić coś w stylu "mikroserwisów", a tak właśnie jest. Ja bym to w środku jeszcze trochę ewentualnie podzielił na pakiety/podmoduły żeby oderwać elementy zależne od frameworka (np. kontrolery) od logiki biznesowej, ale poza tym jest zupełnie ok. W badzo dużych projektach to co sugerujesz się zwyczajnie nie sprawdza, bo życzę powodzenia z pakietem controllers który ma powiedzmy tysiąc klas ;]
Zresztą zastanów się co sie dzieje jak coś modyfikujesz -> zwykle interesuje cię jeden z modułów i chcesz dodać np. nowy mapping kontrolera, nowe serwisy, nowe klasy domenowe itd, ale wszystko to związane jest z jednym modułem i w nim możesz pracować.
Jeśli ktoś zrobi tak jak sugerujesz to nagle modyfikuje każdy z modułów aplikacji, bo każdy zawiera mały kawałek. To się sprawdza dla małej aplikacji, albo takiej która ma mocno spójne funkcjonalności.

Wracając do wątku:

1. Nie ma prostej zasady, ot wypakowujesz tam gdzie potrzebujesz :)
2. Zwykle potrzebujesz jakiejś "translacji" błędu. Rzucenie userowi do UI jakiegoś NullPointerException to zły pomysł bo ani on nie wie co jest nie tak ani ty, kiedy dostaniesz ticked z bug reportem. Zresztą często wcale nie chcesz od razu "wywalić" całej akcji bo poleciał jakiś wyjątek. Z wielu wyjątków można jeszcze zrobić jakieś recovery...

Dzięki za szybką i wartościową krytykę! :) Jak wspominałem jestem raczej początkującym programistą, który uczy się aby zostać juniorem ^^ :)

scibi92 napisał(a):

1)Zamiast dzielić projekt "pionowo" podzieliłbym poziomo, czyli pakiety repository, entity, controller etc.

Jeszcze 2 tygodnie temu miałem katalogi typu repositories, domain, controllers..., ale to się raczej sprawdza przy małych projektach. W rozbudowanych systemach widywałem katalogowanie po modułach user, project itp. Chodzi mi o to, że ciężko byłoby się odnaleźć w projekcie przy większej ilości klas.

scibi92 napisał(a):

2)Po co kwiatki w stylu UserPersistence ? Właściwie to widze że to głównie wywołanie operacji z Repository, ale jest na przykład

Kopiuj

private String getLoggedUser(){
        return SecurityContextHolder.getContext().getAuthentication().getName();
    }

Do metody ze zmianą hasła powinno być przekazane hasło i login. To chyba jednak ma byc obiekt od logiki? W takim razie nazwa niezbyt trafna. Zreszta własnie zauważyłem że nazwałeś jest z adnotacją @Service. Słabo, zmienić nazwe na UserService

Używanie nazewnictwa typu UserService, ProjectService jest według mnie błędne. Co te nazwy oznaczają? Do takich klas można wszystko przypisać od persystencji do powiedzmy zmiany hasła użytkownika. Takie coś raczej jest błędne jeśli chodzi o zasady SOLID, a dokładniej SRP. Ubolewam nad faktem, iż mam klasę ProjectManager do której teoretycznie też można wszystko wrzucić. Aktualnie szukam fajnego sposobu, aby ten problem rozwiązać.

Stworzyłem klase UserPersistence (która jest serwisem) do komunikacji z repozytorium, tym samym odciąż klasę typu ProjectManager od posiadania metod CRUD'a. Oczywiście na siłę mógłbym mieć komunikacje bezpośrednią z repository, ale gdy chciałbym wysłać dane do kontrolera to tylko mógłbym przez repository, a samo w sobie to jest błąd gdyż kontroler nie powinien mieć dostępu bezpośredniego do repozytorium.

@Shalom, @JustLive: oczywiście masz racje, dlatego ja jestem za podziałem tez na moduły, ale nie chodzi o to żeby jeden moduł miał jedną encję ;]
Można to rozdzielic na "mikroserwisy" i nawet zrobić z tego moduły mavenowe, ale powinno to być jednak troche większe

Używanie nazewnictwa typu UserService, ProjectService jest według mnie błędne. Co te nazwy oznaczają? Do takich klas można wszystko przypisać od persystencji do powiedzmy zmiany hasła użytkownika. Takie coś raczej jest błędne jeśli chodzi o zasady SOLID, a dokładniej SRP

Masz rację. Dlatego nie powinno być klas np. UserService tylko UserRegisterService, gdzie masz obsługę rejestracji. Tworzenie serwisów per encja też mi nie odpowiada (no chyba że rzeczywiście jest tam jedna czy 2 metody). UserPersistence to jakas dla mnie średnio dobra nazwa

Shalom napisał(a):

1. Zwykle potrzebujesz jakiejś "translacji" błędu. Rzucenie userowi do UI jakiegoś NullPointerException to zły pomysł bo ani on nie wie co jest nie tak ani ty, kiedy dostaniesz ticked z bug reportem. Zresztą często wcale nie chcesz od razu "wywalić" całej akcji bo poleciał jakiś wyjątek. Z wielu wyjątków można jeszcze zrobić jakieś recovery...

Zwykle gdy jest szansa wystąpienia NPE to tworze własne wyjątki typu UserNotFoundException bądź ProjectNotFoundException i gdy ten wyjątek zostanie wyrzucony to w przeglądarce wyświetla się komunikat: no a such project, z kodem 404. A to jest chyba coś co chcę osiągnąć :)
Aktualnie z moim bieżącym doświadczeniem zawsze gdy miałem do czynienia z wyjątkami to ...uwaga... robiłem printStackTrace hihihi :)

Wcześniej przed odnalezieniem ten adnotacji używałem try catcha w kontrolerze... ale to nie jest "seksi" rozwiązanie, także zacząłem szukać i szukać, aż znalazłem @ResponseStatus :)

gdyż @ResponseStatus umożliwia mi ustalenie HttpStatus oraz reason

reason nie należy używać. Klient ma obowiązek ignorować ten tekst. Może w Twoim zastosowaniu (nie oglądałem) to pasuje, ale piszę o ogólnych zasadach HTTP, usankcjonowanych przez RFC (Override the HTTP response status text).

Komunikat o błędzie powinien się znaleźć w body albo jakimś dedykowanym nagłówku.

scibi92 napisał(a):

Masz rację. Dlatego nie powinno być klas np. UserService tylko UserRegisterService, gdzie masz obsługę rejestracji. Tworzenie serwisów per encja też mi nie odpowiada (no chyba że rzeczywiście jest tam jedna czy 2 metody). UserPersistence to jakas dla mnie średnio dobra nazwa

UserPersistence to nazwa wymyślona na szybko, żeby rozdzielić dwie klasy :) A zmiana nazwy to już kwestia sekund ^^. Ważne żeby trzymało się wszystko zgodnie z zasadami. Chodzi mi o jakość kodu, żeby był łatwy w utrzymaniu, łatwy w testowaniu itp.
Zdaje sobie sprawę, że to co aktualnie prezentuje to jest poniżej jakiejkolwiek normy :D Jestem samoukiem i uczę się na błędach :) :)

Jeśli chodzi o mikroserwisy to jeszcze nigdy tego nie robiłem. Fakt, bawię się Dockerem i umiem postawić jedynie aplikacje w jednym kontenerze a baze mysql w drugim kontenerze i zmusić żeby aplikacja pobierała dane z bazy, ale to już wszystko :)

To były "mikroserwisy" w sensie potocznym, chodzi o to o żeby zrobić modularyzacje "pionową" i "poziomią",np.

com.eshop
-users
--repository
--service
--constroller
--entity
--dto
-products
--repository
--service

itd. ;)

Zerknij sobie na ten wpis https://spring.io/blog/2013/11/01/exception-handling-in-spring-mvc#global-exception-handling

1. Najlepiej nigdy. Pandora już raz rozpakowała Optionala i do dziś dzień dzieci uczą się o niej w szkole. Pudełko pod tytułem Optional traktujemy jako pudełko i jeżeli chcemy coś zrobić z wartością w środku, to przekazujemy odpowiednią funkcję. Niech rozpakowaniem zajmie się kod, który potrafi to zrobić.

2. To zależy. Jeżeli masz ogarniętą translację błędów na kody, to można sobie odpuścić try/catch i trzeba tylko zadbać, by gdzieś były logowane szczegółowe informacje o błędach. Tak by można było odgrzebać zgłoszenie użytkownika. Jeżeli nie masz tego mechanizmu, to łapanie błędów blisko punktu wystąpienia, obsługa i wyrzucenie po opakowaniu, by przechwycić wyżej w celu nadania odpowiedniego statusu HTTP będzie lepsze.

edit

aż popełniłem https://koziolekweb.pl/2017/08/07/kiedy-wypakowac-optional/