Wasze opinie o typie Optional<> jako argument w metodzie?

Witam
Co myślicie o używaniu typu optional w sygnaturze metody ? Czy to jest błąd czy może dobre podejście ?

Często się z tym spotykam na code review i zawsze jedna jak i druga strona ma konkretne argumenty odnośnie tego że wolno/nie wolno lub nawet trzeba
Taki przykładowy kodzik:

public List<ToolProject> getFiltered( String projectId, Long toolId) {

// jakiś kodzik wyżej

    if (toolId != null) {
        externalSystemsFiltered = externalSystemsFiltered.stream()
                .filter(externalSystem -> externalSystem.getId().equals(toolId))
                .collect(Collectors.toList());
    }

//jakiś kodzik niżej

czy może lepiej tak:

public List<ToolProject> getFiltered( String projectId, Optional<Long> toolId) {

// jakiś kodzik wyżej

    if (toolId.isPresent()) {
     //costam costam
    }

//jakiś kodzik niżej

Optional lepszy niż null. A Option z Vavr lepszy niż Optional

BTW w tym kodzie externalSystem.getId() zwraca Long czy Optional<Long>

BTW2 coś takiego działa? Nie pamiętam tych metod Javowych

    externalSystemsFiltered = toolId
      .map ( toolIdNotNull ->
        externalSystemsFiltered.stream()
                .filter(externalSystem -> externalSystem.getId().equals(toolId))
                .collect(Collectors.toList())
       )
      .getElse(externalSystemsFiltered);

Personalnie unikam. W twoim przypadku:

1. Zamiast sprawdzać null lepiej zrobić Optional.ofNullable() (przykład pierwszy)
2. Przykład drugi nawet nie zadziała, bo w warunku filtrowania funkcji jest porównywanie longa do Optionala

Przykład drugi nawet nie zadziała, bo w warunku filtrowania funkcji jest porównywanie longa do Optionala

Czyli jednak:

    externalSystemsFiltered = toolId
      .map ( toolIdNotNull ->
        externalSystemsFiltered.stream()
                .filter(externalSystem -> externalSystem.getId().equals(toolIdNotNull))
                .collect(Collectors.toList())
       )
      .getElse(externalSystemsFiltered);

Panowie, usunąłem to co jest w 2 przykładzie, bo nie o to chodzi.

Chodzi o samą koncepcję, dawania Optional w argumencie metody. Ktoś ma jakieś argumenty za albo przeciw ?

Oczywiście, że przeciw. Jeśli akurat boli nas słabość javy że nie ma defaultowych parametrów, to zawsze można napisać 2 przeciążone metody (jedna z dwoma argumentami, druga z trzema).

Optional w argumencie zdecydowanie lepszy od null. I dla mnie całkiem normalna sprawa.

1. A jakieś Optional.ofNullable to już jakaś mocne dragi. Rypiemy nulla w API, żeby go potem na Optional zamieniać
2. Jak @KamilAdam napisał - polecam Vavr i Option z Vavr
3. Jak już się używa Option/al to nie używa się isPresent tylko map, flatMap , forEach, getOrElse itp.
4. A najbardziej polecam nie używać Javy tylko języka, gdzie problem null jest rozwiązany (na jvm Kotlin lub Scala 3).

@dawiddawido: nie widzę żadnego sensu w tego typu zabiegu. O ile część monad np. Lazy ma sens, o tyle nie rozumiem dlaczego i po co miałbym ładować Optional w parametrze.

@jarekr000000: ofNullable to zwykła zamiana null -> none. Niespecjalnie widzę co w tym złego, tzn. idealnie nie powinno się z tego korzystać, ale masa bibliotek używa nulla jako "brak wyniku" - i od tego nie uciekniesz, nawet i w Kotlinie będziesz musiał to jakoś obsłużyć.

wartek01 napisał(a):

@dawiddawido: nie widzę żadnego sensu w tego typu zabiegu. O ile część monad np. Lazy ma sens, o tyle nie rozumiem dlaczego i po co miałbym ładować Optional w parametrze.

@jarekr000000: ofNullable to zwykła zamiana null -> none. Niespecjalnie widzę co w tym złego, tzn. idealnie nie powinno się z tego korzystać, ale masa bibliotek używa nulla jako "brak wyniku" - i od tego nie uciekniesz, nawet i w Kotlinie będziesz musiał to jakoś obsłużyć.

Ale wiesz, że ja pisałem w Kotlinie, w kotlinie technicznie nie używa się ofNullable. Tylko, dostaje się T!, które albo od razu konwertuje się na T (czyli nie nullowalne, bo wiesz), albo T? bo wiesz, że nullowalne. I w sumie byłem zaskoczony jak mało bibliotek zmusza mnie do walcowania się z tym (bo są nie nullowe alternatywy.).
Przyznaje, że w swoim kodzie w kotlinie używam często Optionzamiast T? :-) (ale to nie dotyczy korzystania z bibliotek Javowych).

jarekr000000 napisał(a):

1. A jakieś Optional.ofNullable to już jakaś mocne dragi. Rypiemy nulla w API, żeby go potem na Optional zamieniać

Tu widać iż już chyba zapomniałeś jak działa Java XD Jak zrobisz Optional.of(myVar) i akurat myVar był nullem to mamy exception zamiast Optionala. Co za chora bioblioteka XD
Więc w zasadzie zawsze trzeba pisac Optional.ofNullable(myVar) XD

BTW w vavrze już jest normalnie Option.of(myVar) nie rzuca exceptiona

https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#of-T-
https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#ofNullable-T-
https://www.javadoc.io/static/io.vavr/vavr/0.9.2/io/vavr/control/Option.html#of-T-

Oczywiście, że tak. Problemem jest Java i jej legacy, które sprawia, że używanie typów takich jak optional w różnych kontekstach jest upierdliwe, bo nie daje to pewności, że ktoś i tak nie użyje null w innym miejscu.

Same Optionale jako argument nie są też tak bardzo przydatne jak w return value, bo są często niepotrzebne, jeśli analogiczny kod da się osiągnać poprzez przeniesienie logiki sprawdzenia isPresent gdzieś wyżej

Często się z tym spotykam na code review i zawsze jedna jak i druga strona ma konkretne argumenty odnośnie tego że wolno/nie wolno lub nawet trzeba

No to ciśnij a jak nie to niech spi******.

KamilAdam napisał(a):

jarekr000000 napisał(a):

1. A jakieś Optional.ofNullable to już jakaś mocne dragi. Rypiemy nulla w API, żeby go potem na Optional zamieniać

Tu widać iż już chyba zapomniałeś jak działa Java XD Jak zrobisz Optional.of(myVar) i akurat myVar był nullem to mamy exception zamiast Optionala. Co za chora bioblioteka XD
Więc w zasadzie zawsze trzeba pisac Optional.ofNullable(myVar) XD

Akurat pamiętam. Chodzi dokładnie o to, żeby nie mieć nullowalnego (logicznie) myVar, wtedy nie potrzeba Optional.ofNullable. A co inicjalizacji używamy Optional.of lub Optional.empty.

@jarekr000000: tak, wiem jak działa Kotlin. Tyle tylko, że to na co pozwala Kotlin nie sprawdzi się w Javie. Ja sobie nie wyobrażam pisania zamiast T? wszędzie Optional<T>, wolę eliminować nulle odpowiednio wcześniej - tak, żeby potem mieć gwarancję, że wartość istnieje.

@KamilAdam: Optional nie jest tylko do nulli, można zawsze filtrować.

slsy napisał(a):

Same Optionale jako argument nie są też tak bardzo przydatne jak w return value, bo są często niepotrzebne, jeśli analogiczny kod da się osiągnać poprzez przeniesienie logiki sprawdzenia isPresent gdzieś wyżej

To chyba kluczowe zdanie. Bo jeśli używamy Optionalla tak samo jak null. Robiąc tak samo ify (tylko z nieco innym warunkiem isPresent zamiast !=null) to wtedy faktycznie ten Optional za dużo nie daje.
Jedna prosta zasada - nigdy nie używaj get() na Optionallu (co także powoduje, że isPresent też wypada) powoduje, że Optional nabiera sensu.

wartek01 napisał(a):

@jarekr000000: tak, wiem jak działa Kotlin. Tyle tylko, że to na co pozwala Kotlin nie sprawdzi się w Javie. Ja sobie nie wyobrażam pisania zamiast T? wszędzie Optional<T>, wolę eliminować nulle odpowiednio wcześniej - tak, żeby potem mieć gwarancję, że wartość istnieje.

Jak się ma to do tego:

wartek01 napisał(a):

Personalnie unikam. W twoim przypadku:

1. Zamiast sprawdzać null lepiej zrobić Optional.ofNullable() (przykład pierwszy)

Bo to jest dokładnie wprowadzanie null zamiast eliminowania.
Właśnie konsekwentnie trzymając się Optional lub T? w tych miejscach, gdzie faktycznie coś jest opcjonalne - eliminujesz null i tony niepotrzebnych sprawdzań.
Bo możesz w końcu przyjąć, że jak coś jest po prostu typu T to na pewno nie jest null.

jarekr000000 napisał(a):

Bo to jest dokładnie wprowadzanie null zamiast eliminowania.

To jest przykład jak obsłużyć jedną metodę z - jak rozumiem - argumentem, który może być null lub nie. Przykład z wątku jest dla mnie bardzo koślawy - ja bym to zrobił mniej więcej tak:

public void mainCall(SomeWonderfulContext ctx) {
	String projectId = ctx.getProjectId(); // jeśli jest null to powinno od razu rzucić wyjątkiem 
	List<ToolProject> externalSystems = ctx.getOptionalToolId()
		.map(toolId -> getExternalSystemsByProjectIdAndToolId(String projectId, Long toolId))
		.orElseGet(() -> getExternalSystemsByProjectId(String projectId));

	// do some stuff
}

private List<ToolProject> getExternalSystemsByProjectId(String projectId) {
	// …
}

private List<ToolProject> getExternalSystemsByProjectIdAndToolId(String projectId, Long toolId) {
	// …
}

Takie podejście jest dla mnie czytelniejsze niż sterowanie przepływem przez przekazywanie Optional.

wartek01 napisał(a):

jarekr000000 napisał(a):

Bo to jest dokładnie wprowadzanie null zamiast eliminowania.

To jest przykład jak obsłużyć jedną metodę z - jak rozumiem - argumentem, który może być null lub nie. Przykład z wątku jest dla mnie bardzo koślawy - ja bym to zrobił mniej więcej tak:

...

Takie podejście jest dla mnie czytelniejsze niż sterowanie przepływem przez przekazywanie Optional.

Co do konkretnego przypadku - zgoda, ma sens.
Jakkolwiek,

1. Jeśli getExternalSystemsByProjectId i getExternalSystemsByProjectIdAndToolId miałyby dużo wspólnego kodu to może jakiś
   prywatny getExternalSystems(Strin projectId, Optional<Long> toolId) miałby sens (zalezy od tego ile razy i jak toolId jest używane w środku).
2. W normalnych językach, gdzie mamy domyślne argumenty, jedno getExternalSystems(projectID: String, toolId : Option<Long> = None) jest zwykle najsensowniejszym rozwiązaniem.

Tutaj jest tylko jeden opcjonalny argument więc jest prosto bo 2^1 równa się 2. A jakby były cztery opcjonalne argumenty to już mamy 2^4 czyli 16 XD

Więc w tym przypadku jak najbardziej możeby zrobic dwie metody, ale mam niestety kod gdzie mam trzy opcjonalne parametry i nie moge sobie napisać 9 metod. Ok niby mogę, ale IMHO to gorsze rozwiazanie. Zresztą te nazwy. myMethodWithAAndBWithoutC(a: A, b: B) XD

KamilAdam napisał(a):

Więc w tym przypadku jak najbardziej możeby zrobic dwie metody, ale mam niestety kod gdzie mam trzy opcjonalne parametry i nie moge sobie napisać 9 metod. Ok niby mogę, ale IMHO to gorsze rozwiazanie. Zresztą te nazwy. myMethodWithAAndBWithoutC(a: A, b: B) XD

Jak masz specjalny monitor panoramiczny do javy to takie metody to nie problem.

@KamilAdam: raczej nieczęsto spotykam się z funkcjami przyjmującymi więcej niż trzy argumenty. Przy czym taka funkcja z trzema opcjonalnymi parametrami jest już ciężka do testowania, bo jak zauważyłeś - complexity leci do góry - a to tylko przy prostym jest/nie ma, mogą dojść inne warunki brzegowe.

Być może w twoim przypadku faktycznie nie da się tego uprościć, ale raczej z mojego doświadczenia wynika, że z 90% kodu po prostu da się przepisać tak, żeby było dobrze. Trudno ocenić bez kodu i kontekstu - bo jeśli kod wygląda w tym stylu (nawet jeśli jest na optionalach):

public void doSomething(String nonNullable, Long mayBeNull, Long mayBeNullToo, Long itAlsoCanBeNull) {
	if (allNulls(mayBeNull, mayBeNullToo, itAlsoCanBeNull)) {
		// do something
	} else if (allNulls(mayBeNull, mayBeNullToo) && allNonNulls(itAlsoCanBeNull)) {
		// do something completely different
	} else if // … I tak dla wszystkich kombinacji 
	else {
		System.exit(-1); // this should NEVER happen
	}
}

to bym autorowi kolejkę postawił za optymizm.

wartek01 napisał(a):

@KamilAdam: raczej nieczęsto spotykam się z funkcjami przyjmującymi więcej niż trzy argumenty. Przy czym taka funkcja z trzema opcjonalnymi parametrami jest już ciężka do testowania, bo jak zauważyłeś - complexity leci do góry - a to tylko przy prostym jest/nie ma, mogą dojść inne warunki brzegowe.

Być może w twoim przypadku faktycznie nie da się tego uprościć, ale raczej z mojego doświadczenia wynika, że z 90% kodu po prostu da się przepisać tak, żeby było dobrze. Trudno ocenić bez kodu i kontekstu - bo jeśli kod wygląda w tym stylu (nawet jeśli jest na optionalach):
...

True. Ale czasem bywa, że całkiem ładnie się upraszcza (jak się ma szeroki monitor):

public List<Stuff> doSomething(Optional<String> group, Optional<String> name, Optional<Long> version) {
 http.get("/service"
   + group.map(groupName -> "/"+groupName).orElse("")
   + name.map(n -> "/" + n).orElse("")
   + version.map(v -> "?version="+v).orElse("")
  )...
}

@jarekr000000: akurat ten przykład mi się nie podoba bo wolę jednak napisać/skopiować (te dostępne w bibliotekach nie podobają mi się) HttpQueryBuilder czy coś w tym stylu.

Niemniej zgadzam się, że przekazanie Optional może mieć sens. Po prostu staram się tego typu konstrukcji unikać gdzie mogę i zazwyczaj mi to wychodzi.

A nie martwicie się że pierwsze co musicie zrobić w metodzie z argumentem jako optional to sprawdzenie

public List<ToolProject> getFiltered( String projectId, Optional<Long> toolId) {

if(toolId != null) {
.... // bo ryzykujecie że poleci NPE jak się nie zabezpieczycie
}

co totalnie wypacza idee optionali ?

Może nie "pierwsze co musiecie zrobić" - ale na pewno musicie zrobić to przed wywołaniem metod optionala. Generalnie widze tendencje, że jak raz to się dopuści w kodzie, optional jako argument - to to się rozlewa na kod, klasy i package i potem stan jest przenoszony między obiektami a w pewnym momencie, legacy kodzie cieżko się połapać czy ten argument jest bezpieczny czy nie. I skoro musicie już robić tego null checka na optionalu, to po co w ogóle go przekazywać jako argument.

Edit. Dodatkowo, u osób które jako argumenty przekazują optionale w parze często idzie taka kontynuacja:

public List<ToolProject> getFiltered( String projectId, Optional<Long> toolId) {

if(toolId != null) {
.... // bo ryzykujecie że poleci NPE jak się nie zabezpieczycie

if(toolId.isPresent()) {
....
}

}

MateInf napisał(a):

A nie martwicie się że pierwsze co musicie zrobić w metodzie z argumentem jako optional to sprawdzenie

public List<ToolProject> getFiltered( String projectId, Optional<Long> toolId) {

if(toolId != null) {
.... // bo ryzykujecie że poleci NPE jak się nie zabezpieczycie
}

co totalnie wypacza idee optionali ?

Nie, nie musimy i nie powinniśmy, faktycznie to zupełnie wypaczałoby sens optionali.

Może nie "pierwsze co musiecie zrobić" - ale na pewno musicie zrobić to przed wywołaniem metod optionala. Generalnie widze tendencje, że jak raz to się dopuści w kodzie, optional jako argument - to to się rozlewa na kod, klasy i package i potem stan jest przenoszony między obiektami a w pewnym momencie, legacy kodzie cieżko się połapać czy ten argument jest bezpieczny czy nie. I skoro musicie już robić tego null checka na optionalu, to po co w ogóle go przekazywać jako argument.

Nie, nie nie - jak wyżej cały sens optionali polega na tym, żeby uprościć kod, nie robić bezsensownych null checków. Jeśli nie ma optionalli to kończy się tym, że mamy tony null checków, 90% z nich zupełnie bez sensu, w miejscach gdzie nulla nigdy nie będzie (ale skoro nie wiadomo to się zabezpieczamy).

Faktycznie po wprowadzeniu Optionale się rozlewają. Za to znikają null checki - na tym to polega.

Tu jeszcze jedno, wbrew pozorm NPE to nie jest problem, NPE to prawdziwy przyjaciel programisty, pokazuje, że ktoś coś zjeb*** i pozwala zatrzymać program zanim zrobi jakieś większe szkody. Jeśli metoda przyjmuje optional, a ktoś przekazał null to znaczy, że ktoś naprawdę zepsuł - i lepiej to zatrzymać.

Osobiście jestem neutralny w tym temacie (uważam, że walka z nullami w Javie to misja straczeńcza), ale dla kompletności powinniście również uwzględnić intencję projektantów Javy, którą Brian Goetz wyraził przykładowo w tym wpisie:

Of course, people will do what they want. But we did have a clear intention when adding this feature, and it was not to be a general purpose Maybe type, as much as many people would have liked us to do so. Our intention was to provide a limited mechanism for library method return types where there needed to be a clear way to represent "no result", and using null for such was overwhelmingly likely to cause errors.
For example, you probably should never use it for something that returns an array of results, or a list of results; instead return an empty array or list. You should almost never use it as a field of something or a method parameter.
I think routinely using it as a return value for getters would definitely be over-use.
There's nothing wrong with Optional that it should be avoided, it's just not what many people wish it were, and accordingly we were fairly concerned about the risk of zealous over-use.

https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555

nie100sowny napisał(a):

Osobiście jestem neutralny w tym temacie (uważam, że walka z nullami w Javie to misja straczeńcza), ale dla kompletności powinniście również uwzględnić intencję projektantów Javy, którą Brian Goetz wyraził przykładowo w tym wpisie:

Dokładnie dlatego piszemy na górze, żeby używać Option z Vavra. Nie dość, że nie jest zrypany, to jeszcze nie był pisany przez ludzi z problemami. BG miał jakiś straszny problem - nie chciał aby java stała się językiem funkcyjnym. W efekcie ludzie i tak piszą funkcyjnie tylko, w jeszcze bardziej kalekim języku (przez takie detale).

Dodatkowo, nie wiem czemu w ogóle mielibyśmy się przejmować intencjami twórców javy. Przecież Ci ludzie (akurat nie BG) opracowali takie rzeczy jak Checked Exceptions i klasy java.io.File albo java.util.Date.
To nie oznacza, że powinniśmy tego używać. Czasy się zmieniają. Intencje też się zmieniają.

Słabe. Lepiej mieć dwie osobne metody, i na tym optionalu wywołać ifPresentOrElse().

Jeśli Funckja przyjmuje Optional to siłą rzeczy łamie SRP.

Riddle napisał(a):

Słabe. Lepiej mieć dwie osobne metody, i na tym optionalu wywołać ifPresentOrElse().

Jeśli Funckja przyjmuje Optional to siłą rzeczy łamie SRP.

Zaczepmy się o jakiś przykład, bo to brzmi jak silver bullet z ewangelistów clean codu, który nigdy nie działa.

Powiedzmy taka domena - na szybko cokolwiek wymyśliłem. Mamy jakieś resource poukładane w grupy, np. pliki w folderze. Mamy zdefiniowany taki kontrakt napisany w formie kontrolera springowego. Endpoint ma zwracać najbardziej pasujący do query plik (np. po nazwie) ze wskazanego folderu. Jeżeli nie ma wskazanego folderu to z prywatnych folderów wywołującego go użytkownika.

I co teraz? Kontroler ma bawić się w logikę biznesową i decydować do którego serwisu/fasady/handlera uderzać (np. FindInResourceGroupService lub FindInUserResourcesService)? Jak się zmienia implementacja gdy dojdą kolejne nullowalne parametry teamGroupId, organizationGroupId, a logika ma przeszukiwać po kolei w grupach jeżeli nie są nullami groupId -> teamGroupId -> organizationGroupId

@GetMapping("/x")
public void getMostSuitableResource(
    @RequestParam(required=true) query: String,
    @RequestParam(required=false) groupId: Optional<GroupId>
) {
    // i co, groupId.ifPresentOrElse()?
    // resourceFacade.getMostSuitableResource(query, groupId) podobno łamie SRP, więc nie można.
}

Kontrakt jest narzucony z góry przez frontendowca pracującego od 25 lat w firmie albo przez klienta, więc jest nieruszalny (czyt. nie przyjmuję odpowiedzi że to powinny być 2 endpointy czy cokolwiek innego)

Riddle napisał(a):

Słabe. Lepiej mieć dwie osobne metody, i na tym optionalu wywołać ifPresentOrElse().

Jeśli Funckja przyjmuje Optional to siłą rzeczy łamie SRP.

Piekne. Uogólniłbym na: jeśli funkcja przyjmuje jakikolwiek argument to siłą rzeczy łamie SRP.

Chodzi o samą koncepcję, dawania Optional w argumencie metody. Ktoś ma jakieś argumenty za albo przeciw ?

Nie.

Optional ma m.in sugerować potencjalny error, co ma sens przy zwracanej wartości. Chodzi o intencje.

Sugerowanie że chce się odbierać złą wartość jest bez sensu.
Przy czym zaznaczam że chęć odbierania jej, od bycia w stanie obsłużyć złą wartość to co innego.

Nie wiem jak to napisać ale jakbym widział API typu Math.Sqrt(5) akceptujące NULLable double, to byłoby to dziwne.

Dodatkowo jeżeli metoda potrzebuje do działania X, a ty chcesz aby to było podane w jakiejś nakładce Wrapper<X> to jest to po prostu słaby design.

API powinno mieć w parametrach to, co faktycznie potrzebuje - ale to też zależy od kontekstu

Np. CalculateUsersBlabla raczej powinno przyjmować Usera i z niego wybierać sobie dane do Blabla niż przyjmować Blabla jako arg.
Ale funkcja CalculateBlabla powinna raczej przyjmować Blabla zamiast Usera

WeiXiao napisał(a):

Chodzi o samą koncepcję, dawania Optional w argumencie metody. Ktoś ma jakieś argumenty za albo przeciw ?

Nie.

Optional ma m.in sugerować potencjalny error, co ma sens przy zwracanej wartości. Chodzi o intencje.

Co za bzdura.
Optional nie oznacza, żadnego Erroru. None to po prostu kolejna wartość spoza tego co oryginalny typ T przewiduje.

Sugerowanie że chce się odbierać złą wartość jest bez sensu.
...
Dodatkowo jeżeli metoda potrzebuje do działania X, a ty chcesz aby to było podane w jakiejś nakładce Wrapper<X> to jest to po prostu słaby design.

A co jeśli funkcja potrzebuje Wrapper<X>?

Przykład:
Order.deliver(deliveryAddress : Option<Address>)
wysyłamy na adres pod którym ktoś się zarejestrował, chyba że podał alternatywyny adres dostawy.