Nadmiarowe użycie optionali w springowym repository

0

Cześć, jestem aktualnie w projekcie w którym każda metoda w interfejsach repository jest okraszona Optionalem, co bardzo mi się nie podoba bo w każdym miejscu gdzie pobieram jakieś rekordy muszę robić sprawdzanie czy zapytanie coś zwróciło i jeśli nie, zwrócić kod błędu zapakowany w obiekt Response:

Optional<Company> optCompany = companyRepo.findById(id);

if(!opt.Company.isPresent()){
 return Response.create(ResultCode.COMPANY_NOT_EXIST);
}
Company company = optCompany.get();

problem pojawia się gdy takich sprawdzeń mam na przykład 3 co bardzo zaśmieca kod moim zdaniem i metodka która powinna mieć 5 linijek nagle ma 1500. W niektórych przypadkach takie optionale są przydatne, na przykład gdy pobieramy rekord który może w wyniku działań jakiegoś innego użytkownika zostać usunięty, ale w przypadku gdy chodzi o Company to usunięcie takiego rekordu to już grubsza sprawa i myślę że nie trzeba tego za każdym razem sprawdzać. Z drugiej strony usunięcie tego optionala spowoduje, że musiałbym zmienić kod w jakichś 50 miejscach (x2 bo testy, x <ilość zapytań do przerobienia>). poza tym nie chcę za bardzo ruszać starego kodu. Czy dałoby się to jakoś obejść w taki sposób żeby dorobić kolejny interfejs jakoś? W którym byłyby te same metody typu findById ale zwracały by samą encję bez Optional? Jak mogę to ugryźć?

2

Czy widzisz różnicę między nullem a optionalowym None? I czy korzystasz w ogóle z flatMap ?

5

o_O przecież to co napisałeś nie ma sensu w ogóle. Skoro repository zwraca optionala to znaczy że może ci nie zwrócić wyniku i musisz coś z tym zrobić!

ale zwracały by samą encję bez Optional

No ok ale co mają zrobić jak jednak takiej wartości w bazie nie ma? :D Mają ci ją wyczarować? xD Nikt ci nie broni zrobić wszędzie optional.get() i yolo, tylko ze to po prostu nie będzie działać, bo kod się wywali przy pierwszej wartości której w bazie nie ma.

3

Problem w tym, że używasz Optionala jak zwykłą nullowalną wartość, więc rzeczywiście uzysk z tego designu jest żaden. Popatrz, w jaki sposób z Optionala korzysta Twój zespół - komunikuj się!

2

Jeśli twoje repo to CRUDowe Spring repository, to by default one zwracają Optionale, ale to jest dobra praktyka - jak zrobisz findbyId(333L) i akurat nie ma rekordu z id 333, to co powinno się stać? Powinieneś dostać nulla i narazić się na NPE? Czy może wyjątek? Optional w takich szukaniach to dobre podejście, bo za każdym razem musisz obsłużyć przypadek, kiedy nie ma takiego rekordu znalezionego w bazie. Żeby skrócić linijki kodu, możesz zrobić coś takiego:

Company company = companyRepo.findById(id)
                                            .orElseThrow(() -> new RecordNotFoundException(ResultCode.COMPANY_NOT_EXIST));

gdzie RecordNotFoundException byłby twoim customowym exceptionem, w którym podajesz swój enum jako argument. Wyżej, mógłbyś wywołać tę metodę w try catchu, i jeśli złapałbyś wyjątek, to zwracasz Response.create(exception.getMessage()). Można podejść do tego w jeszcze większej skali - jeśli twój wyżej podany kod implementuje jakiś generyczny interfejs, to możesz założyć aspect otaczający wywołanie danej metody interfejsu, w którym robisz try/catch i jeśli poleci jakiś wyjątek, to wtedy zwracasz swój Response.

To co podałem to trochę workaround - mogą potem pojawić się problemy z transakcyjnością, jeśli nie będziemy znać kolejności aspektu twojego oraz Transactional Springowego. No i exception driven development wprowadza opóźnienia dla JVM (tworzenie stacktrace itp.).

tl;dr Przekonaj się do Optionali

2

@grzechup:

Company company = companyRepo.findById(id)
.orElseThrow(() -> new RecordNotFoundException(ResultCode.COMPANY_NOT_EXIST));

I to jest bardzo zła praktyka w 90% przypadków.
Zamiast tak robić to można
1)wykorzystać ifPresent
2)wykorzystać map
3)wykorzystać orElse

Jak zwrócisz Optionala z controllera to chyba nawet Spring ogarnie i zwróci 404 jak optional będzie pusty

@Pinek
A Ty zamiast takie głupoty opowiadać to się doucz

0
scibi92 napisał(a):

@Pinek
A Ty zamiast takie głupoty opowiadać to się doucz

A doczytałeś chociaż mój post do końca? -,-

0
Shalom napisał(a):

o_O przecież to co napisałeś nie ma sensu w ogóle. Skoro repository zwraca optionala to znaczy że może ci nie zwrócić wyniku i musisz coś z tym zrobić!

ale zwracały by samą encję bez Optional

No ok ale co mają zrobić jak jednak takiej wartości w bazie nie ma? :D Mają ci ją wyczarować? xD Nikt ci nie broni zrobić wszędzie optional.get() i yolo, tylko ze to po prostu nie będzie działać, bo kod się wywali przy pierwszej wartości której w bazie nie ma.

chodzi o to, że w requeście przychodzi id użytkownika, zazwyczaj po tym id'ku pobieram jego obiekt encji Profile - w tym przypadku jestem pewny że w 99% przypadków ten obiekt istnieje bo użytkownika nie da się usunąć z poziomu aplikacji, więc NPE będzie się rzadko zdarzać (nawet jeśli się zdarzy, to trudno, poleci NPE i tyle - chyba, że się mylę i to wcale nie trudno?). To samo jest z Company, pobieram takie które przyszło w requeście, a request jest wysyłany ze strony po uprzednio pobranych firmach z bazy. Firmy też nie da się usunąć, także encja na 99% będzie w bazie. Wydaje mi się, że ten 1% po prostu nie jest wart tego żeby zaśmiecać metodę dodatkowymi 5-6 linijkami kodu dla każdego obiektu.
Tutaj znalazłem ciekawy artykuł na ten temat: https://tuhrig.de/anti-pattern-dont-use-optionals-for-data-repositories/

Charles_Ray napisał(a):

Problem w tym, że używasz Optionala jak zwykłą nullowalną wartość, więc rzeczywiście uzysk z tego designu jest żaden. Popatrz, w jaki sposób z Optionala korzysta Twój zespół - komunikuj się!

Problem w tym, że to jest kod który zastałem w tym projekcie i wcale mi się nie podoba :)

5

nawet jeśli się zdarzy, to trudno, poleci NPE i tyle - chyba, że się mylę i to wcale nie trudno?

xD Powiedz to swojemu leadowi, że czasem aplikacja jebnie z NPE, ale to rzadko :D :D A już szczególnie w połowie jakiejś transakcji, albo jak już jakieś operacje były wykonanie w systemie i nagle masz niespójny stan czy coś.

Anyway, tak jak napisałem, nikt ci nie broni robić na pałe optional.get() i yolo. Oczywiście obawiam się ze twój zespół nie podzieli tego entuzjazmu :D :D

2

Brzmi troszkę tak jakbyś nawet nie próbował zrozumieć po co te optionale tylko uważasz, że tak jest źle i koniec. Standardowa praktyką, nawet w javie jest zwracanie Optionala w takich wypadkach. Pomiń my fakt, że po prostu źle używasz optionali (post 1).

0
DisQ napisał(a):

Brzmi troszkę tak jakbyś nawet nie próbował zrozumieć po co te optionale tylko uważasz, że tak jest źle i koniec. Standardowa praktyką, nawet w javie jest zwracanie Optionala w takich wypadkach. Pomiń my fakt, że po prostu źle używasz optionali (post 1).

Ten kod już taki był wcześniej, ja też uważam, że jest brzydko i zastanawiam się nad schludnym rozwiązaniem.

Shalom napisał(a):

nawet jeśli się zdarzy, to trudno, poleci NPE i tyle - chyba, że się mylę i to wcale nie trudno?

xD Powiedz to swojemu leadowi, że czasem aplikacja jebnie z NPE, ale to rzadko :D :D A już szczególnie w połowie jakiejś transakcji, albo jak już jakieś operacje były wykonanie w systemie i nagle masz niespójny stan czy coś.

Anyway, tak jak napisałem, nikt ci nie broni robić na pałe optional.get() i yolo. Oczywiście obawiam się ze twój zespół nie podzieli tego entuzjazmu :D :D

To sprawdzanie jest zawsze na początku metody w serwisie, więc nie będzie przypadku że przerwana zostanie transakcja. Wywaliło by się przed wszystkim.
Optional.get() nie mogę bo sonar krzyczy :D

1
grzechup napisał(a):

Ten kod już taki był wcześniej, ja też uważam, że jest brzydko i zastanawiam się nad schludnym rozwiązaniem.

Ale co jest schludnego w sprawdzaniu na wejściu każdej metody != null? Natomiast, jeżeli odpowiedź jest, że nie muszę sprawdzać za każdym razem, bo sprawdziłem na początku, to co jest w tym schludnego? Musisz za każdym razem sprawdzić, czytając kod, czy gdzieś tam na początku sekwencji wywołań zostały sprawdzone warunki na null. A co jak po drodze zostanie coś zmapowane na null? Tracisz jakąkolwiek możliwość lokalnego rozumowania w metodach, w których korzystasz z jakichkolwiek obiektów. Czy może to NPE jest schludne?

Poprawna odpowiedź, to tak jak wcześniej już Ci pisali - map(), flatMap(), orElse() itd.

2
grzechup napisał(a):
DisQ napisał(a):

Brzmi troszkę tak jakbyś nawet nie próbował zrozumieć po co te optionale tylko uważasz, że tak jest źle i koniec. Standardowa praktyką, nawet w javie jest zwracanie Optionala w takich wypadkach. Pomiń my fakt, że po prostu źle używasz optionali (post 1).

Ten kod już taki był wcześniej, ja też uważam, że jest brzydko i zastanawiam się nad schludnym rozwiązaniem.

Nie sądzę, abyś był przygotowany do "leczenia projektu", nie rozumiejąc tematu głębiej.

2

Ja nie uważam, że zwracanie optionali jest brzydkie. Wręcz zachęcam do tego :)

Twój sposób, czyli zwracanie nulli gdzie popadnie doprowadzi do systemu w którym nikt nie chciałby nic zmieniać bo każdy by się bał. Zmienisz pozornie pierdołe i nagle nullpointery się sypią na lewo i prawo.

Dobra praktyką jest zapewnienie, że jeśli metoda zwraca nieopakowany w Optionala obiekt to nie jest on nullem. Jeśli metoda przyjmuje nie opcjonalny parametr to on nie może być nullem. Jeśli masz cień szansy że ten obiekt może nie istnieć - zwróć Optionala. Jeśli twoja metoda wykonuje dodatkowe operację jeśli dany argument nie jest nullem, niech ta metoda przyjmuje argument opakowany w Optionala. Nie używaj isPresent oraz get. Jeśli to robisz to zapewne znaczy, że masz problem w kodzie i powinieneś sprawdzić co doprowadziło do takiej sytuacji.

0
AnyKtokolwiek napisał(a):
grzechup napisał(a):
DisQ napisał(a):

Brzmi troszkę tak jakbyś nawet nie próbował zrozumieć po co te optionale tylko uważasz, że tak jest źle i koniec. Standardowa praktyką, nawet w javie jest zwracanie Optionala w takich wypadkach. Pomiń my fakt, że po prostu źle używasz optionali (post 1).

Ten kod już taki był wcześniej, ja też uważam, że jest brzydko i zastanawiam się nad schludnym rozwiązaniem.

Nie sądzę, abyś był przygotowany do "leczenia projektu", nie rozumiejąc tematu głębiej.

Napisałem że nie chcę dotykać starego kodu. Zastanawiałem się tylko jak to można lepiej robić w nowym kodzie który piszę, a nie powtarzać ten sam kod, który jest wszędzie.

1

jak to można lepiej robić

Używać map() ;)

1
grzechup napisał(a):
AnyKtokolwiek napisał(a):
grzechup napisał(a):
DisQ napisał(a):

Brzmi troszkę tak jakbyś nawet nie próbował zrozumieć po co te optionale tylko uważasz, że tak jest źle i koniec. Standardowa praktyką, nawet w javie jest zwracanie Optionala w takich wypadkach. Pomiń my fakt, że po prostu źle używasz optionali (post 1).

Ten kod już taki był wcześniej, ja też uważam, że jest brzydko i zastanawiam się nad schludnym rozwiązaniem.

Nie sądzę, abyś był przygotowany do "leczenia projektu", nie rozumiejąc tematu głębiej.

Napisałem że nie chcę dotykać starego kodu. Zastanawiałem się tylko jak to można lepiej robić w nowym kodzie który piszę, a nie powtarzać ten sam kod, który jest wszędzie.

Lekarstwo jest to samo: pogłębić znajomość. Wiem, ze są i po polsku YT (nie mam zabookmarkowanego)

Sam nie jestem w wieku juniorskim, ale zwapnienia mózgu nie mam, i jak bardzo łopatą przywalę, to tam coś nowego wejdzie.

0
grzechup napisał(a):

To sprawdzanie jest zawsze na początku metody w serwisie, więc nie będzie przypadku że przerwana zostanie transakcja. Wywaliło by się przed wszystkim.
Optional.get() nie mogę bo sonar krzyczy :D

Dirty Hack tylko dla świadomych użytkowników (ja dobrowolnie używam tylko w testach) to bezparametrowa metoda orElseThrow() (https://docs.oracle.com/javase/10/docs/api/java/util/Optional.html#orElseThrow()) dodana w Javie 10. Implementacja jest taka sama jak dla metody get(), ale sonar nie krzyczy :P

A na poważnie to jeśli chcesz zrozumieć klasę Optional to tak jak już tu pisano zacznij od metod map, flatMap i ifPresent

2

Czy możemy zmienić temat wątku z Nadmiarowe użycie optionali w springowym repository na W jaki sposób używać Optionali w Javie? ? :)

1

@grzechup: ja zadam ci inne pytanie. Załóżmy na chwilę, że zamiast Optional masz null i możesz dowolnie rzucać NPE.

Czy fragment:

if(!opt.Company == null){
 return Response.create(ResultCode.COMPANY_NOT_EXIST);
}

będzie występować w twoim kodzie?

0

W tym wątku, ale nie wprost na pytanie OP.

zwracany z repozytorium Optional w pełni mi się układa w głowie jako elegancki, i myślę o nim w kategoriach znaleziono / nie znaleziono, ale repo pracowało poprawnie.

A w rzadkiej sytuacji, jak zaszły nieprzewidziane okoliczności, wyjątek? Either mi się nie układa jako elegancki, zresztą wydaje mi się podobny do "dawnych" kodów powrotu, które mogły być ignorowane / niewykorzystane.

Dobrze myślę?

2
AnyKtokolwiek napisał(a):

A w rzadkiej sytuacji, jak zaszły nieprzewidziane okoliczności, wyjątek? Either mi się nie układa jako elegancki, zresztą wydaje mi się podobny do "dawnych" kodów powrotu, które mogły być ignorowane / niewykorzystane.

Nie do końca. Either na poziomie typów mówi Ci o tym co może się zdarzyć 

0
danek napisał(a):
AnyKtokolwiek napisał(a):

A w rzadkiej sytuacji, jak zaszły nieprzewidziane okoliczności, wyjątek? Either mi się nie układa jako elegancki, zresztą wydaje mi się podobny do "dawnych" kodów powrotu, które mogły być ignorowane / niewykorzystane.

Nie do końca. Either na poziomie typów mówi Ci o tym co może się zdarzyć 

OK. w pełni rozumiem formalizm i semantykę.

  1. Tym niemniej mogę nie być / zwykle nie będę przygotowany na mądrą obsługę problemu. Wyjątek który przeleci przeze mnie jest rozsądniejszy.
  2. Springowcy zwracają TYLKO Optional, zakładam że to przemyśleli.
0
AnyKtokolwiek napisał(a):

A w rzadkiej sytuacji, jak zaszły nieprzewidziane okoliczności, wyjątek? Either mi się nie układa jako elegancki, zresztą wydaje mi się podobny do "dawnych" kodów powrotu, które mogły być ignorowane / niewykorzystane.

Dobrze myślę?

Wyjątki też mogą być łatwo ignorowane (nie mogą być ignorowane tylko Errory w Ruscie bo one składają cały wątek). Ale żeby zignorować Either to już trzeba trochę pracy włożyć

3

To że coś jest w springu, to nie jest żaden wyznacznik ;)

0
danek napisał(a):

To że coś jest w springu, to nie jest żaden wyznacznik ;)

Ale w tym punkcie jesteśmy w tym wątku.
Więc JEŚLI wybrano OIptional (a nie Either), abnormalne sytuacje to ...

2

Bo w czystej javie nie ma Eithera, a jest Optional. Wydaje mi się, że to był główny powód. Nikt Ci nie broni używać go samemu. Wtedy niestety prawdopodobnie będziesz musiał posiłkować się wyjątkiem (jeśli konieczny) opakowanego w Try

4

Eithera ignoruje się bardzo podobine jak checked Exception. I w sumie jest to dość zbliżony mechanizm, tyle że Exception nie jest wartością (da się niby złapać i przekazać do metody, ale nie wygląda to dobrze...).

Dodatkowo Exception zawsze(*) zawiera stacktrace,nawet jak wcale nam nie jest potrzebny (a to kosztuje)... (choć jest to argument typu "wydajność" i generalnie w 99% kodu nie ma znaczenia).

Generalnie - jeśli (biznesowo patrząc), w zdrowej bazie obiektu może nie być .. to oczywiście jedziemy z Optional , Either. I ten Optional w SpringData to bardzo dobra rzecz.
Jeśli jednak brak obiektu to jest jakiś "end of the world" przypadek , z logiki aplikacji wynika że niemożliwy (bp. np . przed chwiklą go założyliśmy, wczytaliśmy...) to wtedy NullPointerException jest bardzo dobrym rozwiązaniem (jeszcze lepszym jest IllegalStateException ze wskazaniem przyczyny - dużo to nie kosztuje).

Czyli, jeśli ten Optional nam nie jest potrzebny .... to robimy orElseThrow i jest dobrze - zamiecione.
To ogólna zasada wartości akceptowalne / dopuszczalne (Optional = None) na niższym poziomie mogą przejść w Wyjątki (Runtime/Panic) na poziomie wyższym - normalka.

O wiele gorzej jest gdy na niższym poziomie mamy Exception (np.ochydny SQLException czy IOException), który jednak jest spodziewany i na wyższym poziomie musimy opakować go w wartość Try, Either,Optional czy coś innego (Result).

Chwalę twórców SpringData - oni te Optionale tam wprowadzili chyba bawet przed Java 8 (jeszcze z guava).

Jest jeszcze taka możliwośc, że mamy dwie metody:

  • jakiś get,find - który zakłada że obiektu może nie być i zwraca Optional
  • jakiś load, który wali Exceptionem za nas jeśli obiektu nie ma.

Takie coś jest (było?) w Hibernate API ... i powodowało dużo zamieszania.

(* z tym zawsze StackTrace - to nie do końca - są optymalizacje na to, ale to dłuższa historia)

0

Java 11 coś wniosła w temacie?
Że jest nowy isEmpty() się dowiedziałem *), a coś istotnego?

*) z polskiego tutka, który uczy isPresent() i get() - ale na wysokiej pozycji YT

1

Jeszcze jest Optional#stream, ale generalnie podłej sytuacji to nie zmienia. Pamietam, że w Scali Option był o wiele bardziej używalny, możesz porównać API obu klas, żeby przekonać się jak ubogi jest ten Javowy. Jeśli zależy Ci na większej „ekspresji typów” to pewnie Vavr będzie dobrym kierunkiem. Tylko wtedy wchodzisz w Eithery, mapy, flatmapy co tez ma swoją wysoka cenę jeśli chodzi o czytelność. Jak zawsze - trade off.

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