Hej,
Poniższy fragment kodu został odrzucony na code review. Czy możecie mi powiedzieć co jest z nim nie tak? Komentarz recenzenta na razie przemilczę.
private User getUser(long userId) {
User user = userRepository.findById(userId);
if (user == null) {
throw new ResourceNotFoundException("User with id: " + userId + " not found.");
}
return user;
}
Być może musisz zwrócić jakieś DTO, zamiast bezpośrednio obiektu z repozytorium?
Wydaje mi się, że istotny jest kontekst w jakim ta metoda jest tworzona. Gdzie ją tworzysz? Btw. Jaki komentarz dał ? :)
Edit: Sokoloki. Czy tu nie chodzi o prywatny getter? Dane pobrane z bazy prawdopodobnie chcesz przekazać dalej.
Edit2: Optionala możesz użyć.
Swoją droga fajny code review jak Ci nie mówią co jest źle
po co w ogole ta metoda zamiast po prostu uzywac userRepository.findById(userId) :)
Java 
tryFindById zeby od razu bylo widac ze spodziewamy sie faila
@xLatency: kurde żyjemy w 2017 roku, Java 8 ma 3 lata a dalej się nie nauczyli że z DAO/Repository jak zwracamy encje której może nie byc to zwracamy Optional?
Kurde heloł ludzie Java 9 za kilkadziesiąt dni (mam nadzieje :D ) a dalej nie umiecie into Java 8? oO
scibi92 napisał(a):
@xLatency: kurde żyjemy w 2017 roku, Java 8 ma 3 lata a dalej się nie nauczyli że z DAO/Repository jak zwracamy encje której może nie byc to zwracamy Optional?
Kurde heloł ludzie Java 9 za kilkadziesiąt dni (mam nadzieje :D ) a dalej nie umiecie into Java 8? oO
Czasami wole zwyklego null checka niż przeginanie z Optionalami.
Problem zależy od kontekstu , ale kod generalnie useless - SpringData od dawna zwracać może Optionale. Wiec po co robić takie nonsensowne przepakowywanie - jak można u źródła naprawić.
Zamieniasz nulla na wyjątek. Zarówno nulla jak i wyjątek trzeba obsłużyć w logice, która korzysta z tej metody.
Gdzie widzisz wartość z tej konwersji?
Możliwe, że chodzi o uniknięcie duplikacji kodu, ale oznaczałoby, że w ramach klasy często korzystasz z getUser(). Jeśli tak, to może klasa powinna odstawać na wejściu Usera, a nie pobierać go sobie z repozytorium? Nie wiedzę całej klasy, więc bez kontekstu ciężko ocenić czy taka konstrukcja ma jakiekolwiek zalety.
yarel napisał(a):
Zamieniasz nulla na wyjątek. Zarówno nulla jak i wyjątek trzeba obsłużyć w logice, która korzysta z tej metody.
Błąd - wyjątków nie trzeba.
Cała zaleta wyjątków (której niestety wielu nie rozumie) polega na tym że ich nie trzeba od razu obsługiwać.
Nawet nie jest to zalecane.
vpiotr napisał(a):
yarel napisał(a):
Zamieniasz nulla na wyjątek. Zarówno nulla jak i wyjątek trzeba obsłużyć w logice, która korzysta z tej metody.
Błąd - wyjątków nie trzeba.
Cała zaleta wyjątków (której niestety wielu nie rozumie) polega na tym że ich nie trzeba od razu obsługiwać.
Nawet nie jest to zalecane.
Mozna rzucac wyjatek i lapac w exception mapperze czy tam controlleradvice i zamieniac na response.
Bzdura że wyjątek trzeba obslugiwac w samym kodzie(chyba że to tzw. checked exception). Możesz dodać w Springu ExceptionHandlery ktore zwrocą ci np. kod błędu albo przekierują na inną stronę
ale wystarczy że masz taki interceptor a gdy go zarejestrujesz to gdziekolwiek wyrzucony błąd trafi do tego interceptora jeśli sam tego nie obsłużysz
vpiotr napisał(a):
yarel napisał(a):
Zamieniasz nulla na wyjątek. Zarówno nulla jak i wyjątek trzeba obsłużyć w logice, która korzysta z tej metody.
Błąd - wyjątków nie trzeba.
Cała zaleta wyjątków (której niestety wielu nie rozumie) polega na tym że ich nie trzeba od razu obsługiwać.
Nawet nie jest to zalecane.
Może mam spaczone spojrzenie, ale wg mnie wyjątek służy do zasygnalizowania nietypowej sytuacji i jest to tylko mechanizm (podobnie jak mechanizmem jest zwracanie kodu błędu przez funkcję). Jak coś informuje o sytuacji nietypowej, to trzeba to obsłużyć, ale gdzie i w jaki sposób, to zależy od sytuacji ;-)
Czy to będzie wpis do logu, czy wyświetlenie użytkownikowi komunikatu, czy zdefiniowanie jakiegokolwiek handlera - jest to nadal obsługa.
Wracając do przykładu, metoda jest prywatna, więc jak zmieni się implementacja getUser i nie będzie rzucanego wyjątku, to jest sens go obsługiwać gdzieś "wyżej" ? :)
-- edycja:
Dodam jeszcze, że w przypadku braku użytkownika rzucanie wyjątku o wdzięcznej nazwie ResourceNotFound razi w oczy. Czemu nie UserNotFound?
Krzywy kr napisał(a):
vpiotr napisał(a):
yarel napisał(a):
Zamieniasz nulla na wyjątek. Zarówno nulla jak i wyjątek trzeba obsłużyć w logice, która korzysta z tej metody.
Błąd - wyjątków nie trzeba.
Cała zaleta wyjątków (której niestety wielu nie rozumie) polega na tym że ich nie trzeba od razu obsługiwać.
Nawet nie jest to zalecane.Mozna rzucac wyjatek i lapac w exception mapperze czy tam controlleradvice i zamieniac na response.
Jeszcze tylko dodam, ze raczej nie powinno sie rzucac wyjatki na poziomie DAO i lapany gdzies w exception mapperze i zmieniany na response ;)
Tylko gdzies po drodze taki wyjatek z dao powinien byc owrapowany jakims serwisowym.
Sporo bzdur się pojawia. Więc postaram się skonfrontować moje poglądy.
Wyjątek checked rzucamy w sytuacji błędu gdy bezpośredni kod wywołujący jest w stanie zareagować/naprawić/ponowić wywołanie.
Unchecked exception, rzucamy gdy kod wywołujący nie jest w stanie nic zrobić z wyjątkiem.
Wyjątki są kosztowne. Wymagają zbudowania stacktrace. Są rzadkie statystycznie i JIT ich nie optymalizuje. Dlatego prowadzenie logiki wyjątkami jest słabe. Szczególnie gdy nasz system jest pod dużym obciążeniem. Wówczas pojawiają się wyjątki np. Timeouty co dodatkowo zabija system. Czyli w kryzysie wolne wyjątki dodatkowo dobijają system.
Czasem w ENTERPRAJS systemach rzuca się unchecked wyjątki wszędzie. Dzięki czemu mamy mniej ifów. A w jakimś aspekcie/advice/filtrze te wyjątki mapuje na błędy HTTP. O ile nasza aplikacja obsługuje 2 userów na godzinę to można to przeżyć. Nadaje się to do CRUDowych i watstwowych aplikacji. Osobiście jednak odradzam.
Najlepiej projektować kod tak, aby system zwracał wynik zawsze. Przydatne klasy to Optional/Either. RxJava, która zmusza do pomyślenia o błędach.
Pomysł z przepakowywaniem wyjątków to tragedia - techniczny kod który nic nie robi, a jedynie zadowala ego programisty.
Wyjątek per zasób to również niepotrzebny bojilerplate.
W skrócie nie rzucaj wyjątków stosuj Optional/Either. I projektuj system w którym gdy poleci unchecked exception to oznacza to konieczność zmiany środowiska/konfiguracji/kodu. W dobrej aplikacji wyjątki nie latają.
Ten kod może być dobry lub zły - w zależności od architektury.
Sytuacje w zasadzie są możliwe dwie:
a) obiekt jest wymagany
b) obiekt jest opcjonalny
W przypadku (b) gdy jest opcjonalny (bo np. nie wiesz czy podany w ciasteczku user nie został usunięty przez ostatnie 60 dni) to można stosować null lub Optional i gdzieś tam dalej to jakoś obsłużyć logiką biznesową.
W przypadku (a) gdy jest wymagany (bo np. szukasz usera który właśnie został zalogowany albo którego właśnie udało Ci się dodać do bazy) to bzdurą jest tworzenie jakiejkolwiek logiki do obsługi takich sytuacji gdy jednak go nie znaleźliśmy.
Wtedy rzucasz wyjątek i obsługujesz to jako błąd którego nie można przewidzieć, np. "odłączono drukarkę w trakcie drukowania raportu", "dysk ktoś zapełnił procesem idącym równolegle", "ktoś przeciął siekierą kabel sieciowy" albo... "ktoś puścił delete po całej bazie w trakcie procesu".
Gdzie tą obsługę dajesz i na ilu poziomach ten błąd przepakowujesz (o ile wiesz po co to robisz) to już szczegół techniczny.
Ale zwykle nie jest to poziom bezpośrednio sąsiadujący, bo wtedy równie dobrze moglibyśmy się cofnąć do czasów C i kodów powrotu.
A dlaczego ten kod został odrzucony? Jest kilka możliwości:
Kilka zasobów:
https://www.ibm.com/developerworks/library/j-ejbexcept/index.html
(autoreklama) http://www.foundbit.com/pl/zasoby/jezyki/java/zaawansowane/articles/java-wyjatki.html
vpiotr napisał(a):
W przypadku (b) gdy jest opcjonalny (bo np. nie wiesz czy podany w ciasteczku user nie został usunięty przez ostatnie 60 dni) to można stosować null lub Optional i gdzieś tam dalej to jakoś obsłużyć logiką biznesową.
No nie ucz mi dzieci takich rzeczy. Jak obiekt jest opcjoinalny to się używa Optional. null w kodzie biznesowym nie powinien sie pojawiać (awruk!).
(jak ktoś robis jakieś obliczenia na macierzach i walczy o cykle to jeszcze można zdzierżyć).
nie100sowny napisał(a):
Sporo bzdur się pojawia. Więc postaram się skonfrontować moje poglądy.
Wyjątek checked rzucamy w sytuacji błędu gdy bezpośredni kod wywołujący jest w stanie zareagować/naprawić/ponowić wywołanie.
Unchecked exception, rzucamy gdy kod wywołujący nie jest w stanie nic zrobić z wyjątkiem.
Wyjątki są kosztowne. Wymagają zbudowania stacktrace. Są rzadkie statystycznie i JIT ich nie optymalizuje. Dlatego prowadzenie logiki wyjątkami jest słabe. Szczególnie gdy nasz system jest pod dużym obciążeniem. Wówczas pojawiają się wyjątki np. Timeouty co dodatkowo zabija system. Czyli w kryzysie wolne wyjątki dodatkowo dobijają system.
Czasem w ENTERPRAJS systemach rzuca się unchecked wyjątki wszędzie. Dzięki czemu mamy mniej ifów. A w jakimś aspekcie/advice/filtrze te wyjątki mapuje na błędy HTTP. O ile nasza aplikacja obsługuje 2 userów na godzinę to można to przeżyć. Nadaje się to do CRUDowych i watstwowych aplikacji. Osobiście jednak odradzam.
Najlepiej projektować kod tak, aby system zwracał wynik zawsze. Przydatne klasy to Optional/Either. RxJava, która zmusza do pomyślenia o błędach.
Pomysł z przepakowywaniem wyjątków to tragedia - techniczny kod który nic nie robi, a jedynie zadowala ego programisty.
Wyjątek per zasób to również niepotrzebny bojilerplate.
W skrócie nie rzucaj wyjątków stosuj Optional/Either. I projektuj system w którym gdy poleci unchecked exception to oznacza to konieczność zmiany środowiska/konfiguracji/kodu. W dobrej aplikacji wyjątki nie latają.
A odnośnie przepakowywania wyjątków to się nie zgodzę.Jak rzucasz wyjątek bazodanowy i go mapujesz na response to dla mnie jest to błąd architektoniczny. Bo co ma ten wyjątek wspólnego z RESTem ? Wszystko oczywiście zależy od architektury.
Hej,
Dzięki za odpowiedzi. Zarzutem tutaj był null check zamiast użycia optionala.
Dyskusja jest ciekawa, dlatego pozwolę sobie pociągnąć wątek dalej.
Metoda getUser została wprowadzona dlatego że kilka razy w kodzie odpytuję o tego usera w różnych miejscach - dokładnie 3 razy.
Null check był po to, żeby nie rzucać null pointerem do klienta, tylko wysłać mu konkretną informację. Exceptiony są mapowane na response za pomocą ControllerAdvice.
Jeśli teraz zrobię sobie w Spring Data metodę findById:
Optional<User> findById(long id)
To jak byście to obsłużyli w kontrolerze. Załóżmy że dalej mamy naszą metodę getUser
private User getUser(long userId) {
return userRepository.findById(userId).orElseThrow(new ResourceNotFoundException("User with id: " + userId + " not found."));
}
czy może tak:
private User getUser(long userId) {
return userRepository.findById(userId).get();
}
Abstrahując już od tego czy metoda getUser ma w takim przypadku sens czy nie.
A może faktycznie nie robić żadnego checka na nulla i zrobić jak już ktoś proponował :
User findById(long id)
i w kodzie po prostu
User user = userRepository.findById(id);
//operacje na user - możliwe NullPointer
PS. Prosiłbym również o zachowanie jakiegoś minimum kultury osobistej w wypowiedziach, bo niektóre odpowiedzi ocierają się o pyskówkę.
Oczywiście jeżeli mamy Optional to nie powinien być pobrany obiekt bez sprawdzenia czy on istnieje, więcz pobranie opcjonala i od razu używania get to jest bardzo zły kod.
@nie100sowny no nie zgadzam się. Wyobraź sobie że na przykład masz operację logowania, koleś wpisuje zły login. Złe dane wejściowe to też może być powód do rzucenia wyjątkiem i średnio mi się widzi sprawdzanie po iluś metodach czy jest użytkownik czy go nie ma, jeśli będzie rzuczony wyjątek a interceptor to złapie to znacznie zwiększa czytelnośc kodu.
scibi92 napisał(a):
Oczywiście jeżeli mamy Optional to nie powinien być pobrany obiekt bez sprawdzenia czy on istnieje, więcz pobranie opcjonala i od razu używania get to jest bardzo zły kod.
@nie100sowny no nie zgadzam się. Wyobraź sobie że na przykład masz operację logowania, koleś wpisuje zły login. Złe dane wejściowe to też może być powód do rzucenia wyjątkiem i średnio mi się widzi sprawdzanie po iluś metodach czy jest użytkownik czy go nie ma, jeśli będzie rzuczony wyjątek a interceptor to złapie to znacznie zwiększa czytelnośc kodu.
Nie popadajmy w skrajnosc. Mysle, ze @nie100sowny tez nie mial na mysli, zeby zwracac cos ZAWSZE. Chociaz teoretycznie moglbys zamiast rzucac UserNotFoundException to zwrocic jakiegos dummy usera ;) ale w przypadku logowania to raczej sytuacja wyjatkowa. Co innego jak siegamy do bazy po newralgiczne dane a co innego jak wyciagamy jakies zwyczajne dane.
W Golangu nie ma exceptionow. Erorr to value. Mozna zwracac z funkcji multiple values i sprawdzac czy error jest nil. Czego jest pelno w kodzie Go. Jednym sie to podoba. Innym nie ;) ma to jednak swoje zalety.
Krzywy kr napisał(a):
Mysle, ze piszac bardziej funkcyjnie czy reaktywnie to.... koncept javowych wyjatkow zwyczajnie do tego stylu nie pasuje
No tak średnio, bo czasem trzeba łapać wyjątek w lambdzie i trochę słabo to wygląda.
W JEE wygodnie jest oznaczać własne wyjątki adnotacją @ApplicationException, wtedy podczas wystąpienia nie są one ubierane dodatkowo w EJBException
student pro napisał(a):
No tak średnio, bo czasem trzeba łapać wyjątek w lambdzie i trochę słabo to wygląda.
W JEE wygodnie jest oznaczać własne wyjątki adnotacją @ApplicationException, wtedy podczas wystąpienia nie są one ubierane dodatkowo w EJBException
Pod względem wyjątków JavaEE to mistrzostwo świata (biedy).
Wystarczy zapomnieć o tym ApplicationException, i mimo, że Ci się wydaje, że obsługujesz wyjątek w catch to i tak rollback leci (do tego na koniec transakcji poza twoim kodem, żeby było łatwiej).
Do tego nie zawsze, ważne, żeby interceptor go przyczaił (czyli jeśli poleci z wywoałania prywatnej metody to problemu nie ma). Zawsze mnie rozbija jak widze jak ktoś z tym walczy. Tej koncepcji nie da się wytłumaczyć człowiekowi przyzwyczajonemu, że catch robi po prostu catch i wyjątek obsługuje (co za dziwny pomysł w ogóle :-) ).
Tak ogólnie to o wyjątkach - zwłaszcza checked najlepiej juz zapomnieć (tzn. nie tworzyć nowych - co najwyżej żyć z tymi, które są w starym kodzie).
Koncepcja Try, Either lub Optional jest zdecydowanie wygodniejsza/ bezpieczniejsza - mimo, że na pierwszy rzut oka może się to wydawać to samo.
Tu i tu nas kompilator zmusza do "obsługi" problemu jeśli coś z innej metody wyciągamy.
Checked Exception wymusza catch, a optional .map lub .orElse. (Bo .get nie używamy!! jak pisał @Koziołek https://koziolekweb.pl/2017/08/07/kiedy-wypakowac-optional/).
To gdzie jest różnica?
Ano w tzw. referential transparency - Optional (czy inne monady) to po prostu wartość i można z tym pracować jak z każdą zmienną, a Exception to taki dziwny wybuch idący gdzieś poza kodem - zupełnie innym torem.
Gdzie to da się docenić ?
Pierwsze miejsce to Testy - zamiast dziwacznego sprawdzania wyniku z @Test(expected = NullPointerException ) czy innych cudów - robimy
ładne equals :
assertEquals ( Option.none(), repo.getUser("1234");
W testach "parametrycznych", w szczególnośc w Junit 5 to jest super.
Drugie miejsce to fakt, że teraz możemy nasze "wyjątki" nie tylko propagować "w górę" (czyli kto nas wywołał to dostanie), ale również
przekazywać innym !
Załóżmy, że mamy metodę repo.gimmeAllBros() która zwraca listę użytkowników systemu. I robimy jakiegoś batcha, który co noc bierze wszystkich userów i im wysyła spam :-).
Załóżmy, że to dziwny stary system i aby zebrać dane takiego gościa, to trzeba odpytać kilka baz - nie zawsze się udaje. Zwracamy Either dla każdego.
(załadował się ok, albo byl bład wtedy tylko dajemy Identyfikator).
Ta metoda może mieć postać:
List<Either<Integer, Person>> gimmeAllBros();
I mozemy sobie napisać :
List<Either<Int, Person>> all = repo.gimmeAllBros();
spamThem(all);
recalculateTheirMoney(all);
I teraz metoda spamThem może olać to, że niektórzy nie są znalezieni. No bo to tylko spam.
Ale metoda recalculateTheirMoney, która np. nalicza nowy stan konta musi zareagować i wysłać adminowi, że takich użytkowników konta były nie do ogarnięcia (i tu np. identyfikatory).
Czyli w pewnym sensie możemy propagować wyjątek w drugą stronę(w dół wywołań) i teraz to te wywołane metody są odpowiedzialne, za to żeby ewentualnie zareagować.
@jarekr000000:
Try to masz na mysli ten z javaslang?
Tak . Try z JavaSlang. (⋁⋀⋁⎧ ).To na współpracę ze starym kodem który rzuca exceptionami. Try pozwala to opakować (jako tako - nadal będzie troche brzydki).
W nowym kodzie Either lub Option.
jarekr000000 napisał(a):
Tak . Try z JavaSlang. (⋁⋀⋁⎧ ).To na współpracę ze starym kodem który rzuca exceptionami. Try pozwala to opakować (jako tako - nadal będzie troche brzydki).
W nowym kodzie Either lub Option.
A jak proponujesz ubsluzyc wspomniane cos w stylu UserNotFoundException przy logowaniu? ;)