Czy zagnieżdżone ify to zawsze zło?

Muszę zaimplementować kilka scenariuszy i nie wiem jaki sposób będzie poprawniejszym - zagnieżdżone ify czy interfejsy funkcyjne?

boolean doSomething(User user, Long jakiesId) {	
		Optional<User> foundedUser = userRepository.findByLogin(user.getLogin());
		if(foundedUser.isPresent()) {
			if(checkEmailsCompatibility(user)) {
				if(!isAlreadyAssigned(user, jakiesId)) {
				      ......
				}else {
					throw new AlreadyAssignedException(jakiesId);
				}
			}else {
				throw new NotCompatibleEmailAddressesException(user.getLogin());
			}
		}else {
			............
		}
		return true;
	}

Kod wydawał mi się czytelny do czasu kiedy nie musiałem dla każdego niespełnionego warunku dodać else throw new Exception... No i to if(foundedUser.isPresent()) bardzo śmierdzi.
Nie do końca wiem jaka jest poprawna praktyka rzucania wyjątków. Powinienem to robić już w metodach sprawdzających? W sensie jeśli mam ifWhatever() to dla niespełnionego warunku rzucać Exception czy dopiero poziom wyżej, jak w kodzie który wkleiłem?

Z tymi ifami to jak z tym dowcipem Pana Grabowskiego. dla zainteresowanych

Ify to ogólnie zło.
Krok1. Użyj funkcji map i getOrElse, aby pozbyć się tego isPresent. Profit.
Krok2. Zamień Optional na vavr/Option. Profit.
Krok3. Pozamieniaj metody check,is... tak aby zwracały Either<Błąd,User>. Płacz, że nic sie nie kompiluje.
Krok4. Na Option zrób toEither, i teraz lecisz flatMapem wszystko. Nie do powstrzymania - jak Kubica.
Krok5. Nie masz żadnego ifa.

Nie lepiej jedna metode walidacyjna? Ktora bedzie skladac sie z tych mniejszych metod? Jakby user mial 10 pol do sprawdzenia to bys 10x sie zagniezdzal? Oczywiscie podejrzewam ze roziwazanie jarker00000 jest najlepsze :D. Ale nie znajac monad ja bym zrobil to tak ze zrobil sobie funkcje validujaca i skladajaca sie z tych podfunckji validujacych ktore by w przypadku validacji sypaly exceptionami. Tak wiec unikneloby sie zagniezdzenia ifow bo bys mial po jednym ifie w kazdej z metod :)

o takim rozwiązaniu nie pomyślałem a faktycznie jest bardzo czytelne. jak monady mnie przerosną to na pewno wykorzystam. dzięki! :)

Ify to nie zawsze zło. To normalna konstrukcja. W kodzie powinieneś unikać przede wszystkim "drabiny" ifów (chociaż w pojedynczych przypadkach może to być ok) oraz wielokrotnych zagnieżdżeń (nie ważne, czy to będą ify, czy coś innego - np. callbacki), bo takie rzeczy powodują spadek czytelności kodu i trudność w jego utrzymaniu. U Ciebie to już zaczyna się pojawiać. Można to rozwiązać tak, jak Jarek napisał, przepisując to na kod funkcyjny. Nie wiem, czy zawsze będzie to idealne rozwiązanie. Alternatywą może być przerzucenie całego kodu odpowiedzialnego za walidację w osobne miejsce (np. do oddzielnej metody). Wtedy będziesz miał jednego ifa lub jedno wywołanie metody i w lambdzie możesz sobie przekazać kod, który musisz wykonać.

NO PRZECIEŻ TO SIĘ NIE KOMPILUJE!! Dostaję Type mismatch: cannot convert from Either<Object,User> to Either<Exception,User> Próbowałem też z Try<> ale tam to w ogóle tak namieszałem, że potrzebowałem git reset żeby kod działał chociaż trochę.

Either<Exception, User> checkEmailsCompatibility(User user){
		return userRepository.findByLogin(user.getLogin())
				.filter(u -> u.getEmail().equalsIgnoreCase(user.getEmail()))
				.map(u -> Either.right(u)).orElse(Either.left(new NotCompatibleEmailAddressesException(user.getLogin())));
	}

Java. type inference. Fail.
Zamień
.map(u -> Either.right(u)).
na
.map(u -> Either.<Exception,User>right(u)).

Przy okazji:

Either<Exception,T> to w zsadzie Try<T>.
Ale przeważnie lepszy jest
Either<E,T> gdzie E to twoja klasa, która NIE dziedziczy z Exceptiona. Bo wtedy nie musi potencjalnie pamiętać StackTrace - profit.
Na grzyba Ci stack trace - jak to wcale nie jest błąd programu, tylko zwykły problem błędnych danych wejściowych.

... nie wierzę. dzięki!

Nawet przy skromniejszej refaktorce, zakładającej zachowanie starych, dobrych ifów - wystarczyłoby je poodwracać, żeby zredukować zagnieżdżenie.

Rzucenie wyjątku przerywa bieg działania metody, Nie ma zatem potrzeby dodawać po nim jeszcze else.

W miejsce:

if (checkEmailsCompatibility(user)) {
    if (!isAlreadyAssigned(user, jakiesId)) {
        ......
    } else {
        throw new AlreadyAssignedException(jakiesId);
    }
} else {
    throw new NotCompatibleEmailAddressesException(user.getLogin());
}

Wystarczyłoby więc zrobić:

if (!checkEmailsCompatibility(user)) {
    throw new NotCompatibleEmailAddressesException(user.getLogin());
}
if (isAlreadyAssigned(user, jakiesId)) {
    throw new AlreadyAssignedException(jakiesId);
}
......

Fail fast. I już nie ma zbędnych zagnieżdżeń. Jest za to czytelniej. Czy czytelniej niż z konstrukcją typu:

.filter(u -> u.getEmail().equalsIgnoreCase(user.getEmail()))
.map(u -> Either.right(u)).orElse(Either.left(new NotCompatibleEmailAddressesException(user.getLogin())))

itp., to już temat na osobne rozważania ;)

wiciu napisał(a):

W kodzie powinieneś unikać przede wszystkim "drabiny" ifów (...) oraz wielokrotnych zagnieżdżeń...

Dodam jeszcze, żeby unikać ifów na więcej niż kilka linijek.
U mnie czasem piszą ify na 2 ekrany. ;(

Zasadniczne pytanie w całej tej historii: po co te ify? Należy programować w taki sposób aby nie było możliwe wywołanie doSomething w kontekście gdy isAlreadyAssigned zwróci true.

Jak widać jest dużo rozwiązań tego samego problemu :D Dodam że taką walidację można też machnąć dekoratorem:
new CheckEmailCompatibility(new AlreadyAssigned(user, id), user).result(). Aczkolwiek to chyba troszeczkę przerost formy nad treścią w tym przypadku :) Choć dodanie kolejnego warunku staje się dość proste

lubie_programowac napisał(a):

Zasadniczne pytanie w całej tej historii: po co te ify? Należy programować w taki sposób aby nie było możliwe wywołanie doSomething w kontekście gdy isAlreadyAssigned zwróci true.

Dawaj. Pokaż.
(to nie jest złośliwe - dobry pomysł)

TL;DR
Do OP: nie zawsze.
Ale w kodzie który podałeś jest taki fragment:

            }else {
                throw new NotCompatibleEmailAddressesException(user.getLogin());
            }

Który po angielsku nazwałbym "dangling else". Czyli taka sytuacja która wydaje Ci się że powinna zajść jak coś będzie nie tak z adresem email, ale ktoś po drodze może zmienić kod i ta sytuacja "else" będzie już oznaczać coś zupełnie innego.
Tutaj jeszcze jest w miarę jasne co ten else ma robić. Ale co jeśli będzie to mniej czytelne? Dojdzie do ewaluacji pewnej logiki rozmytej. Albo osoba poprawiająca uzna że musi to poprawić albo nie. Nie wiadomo i jest to subiektywne. Jeśli wydzielisz z tego kodu funkcje będzie to bardziej oczywiste.

Either<> potrafi zaciemnić obraz logiki, często lepiej jest zwrócić własny typ albo użyć specyfikacji, która również zalicza się do deklaracji.

W jego przypadku można by napisać:

var specyfication = new AssignedUserSpecyfication(login);
var asignedUser = specyfication.satisfyingItemFrom(this.UserRepository); 

jarekr000000 napisał(a):

Ify to ogólnie zło.
Krok1. Użyj funkcji map i getOrElse, aby pozbyć się tego isPresent. Profit.
Krok2. Zamień Optional na vavr/Option. Profit.
Krok3. Pozamieniaj metody check,is... tak aby zwracały Either<Błąd,User>. Płacz, że nic sie nie kompiluje.
Krok4. Na Option zrób toEither, i teraz lecisz flatMapem wszystko. Nie do powstrzymania - jak Kubica.
Krok5. Nie masz żadnego ifa.

Lekki odkop, ale natknąłem się na taki problem, co jeśli mam ciąg odpytań z repository? np:

    public Either<UserError, User> findById(Long id) {
        return Option.of(findById(id))
                .toEither(UserError.NOT_FOUND);
    }

    public Either<TicketError, User> findById(Long id) {
        return Option.of(findById(appId, deviceId))
                .toEither(TicketError.NOT_FOUND);
    }

i w serwisie:

    public Either<ResponseError, SomeResponse> doBusinessLogic(long ticketId, long userId) {
        return userRepository.findById(userId)
                .flatMap(user -> ticketRepository.findById(ticketId)
                        .flatmap(ticket -> doLogic(user, ticket))
                );
    }
```

w takim przypadku dostaje info że `Incompatible equality contraint: TicketError and ResponseError`

`ResponseError` to interface które implementują konkretne Errory

```java
public interface ResponseError {
    int getHttpCode();
    String getMessage();
}
```

ale jeśli z warstwy repository zwracam interface `ResponseError` w `Either` to problem znika:

```java
    public Either<ResponseError, User> findById(Long id) {
        return Option.of(findById(appId, deviceId))
                .toEither(TicketError.NOT_FOUND);
    }
```

Pytanie, czy to jest poprawne? Czy ResponseError powinien się znajdować w warstwie Dao/Repository? Jeśli nie to czy jest jakieś lepsze rozwiązanie? @jarekr000000 @danek wiem że wy kumacie czacze :P

@Merylin wiesz ze istnieje mapLeft? ;)

Przy małej aplikacji pewnie bym się w to aż tak nie bawił, ale przy czymś, co się rozrasta, to tak jak @DisQ mówi, gdzieś trzeba sobie zrobić smutne mapowanie jednego w drugie. Kotlin jest o tyle fajny, że zrobi sie to extensionami.

Rozwinę to powyższe:

1. bardzo mała aplikacja
   jeden typ błędu - niezbyt eleganckie, ale wystarcza i nie widzę kłopotów

2. większa aplikacja - modularne błędy :
   wykorzystanie mapLeft i jakiegoś mapera błędów między warstwami ( a nawet kilku maperów)

3. eksperyment (lekka alternatywa do poprzedniego)
   błąd w warstie serwisu rest jako Any - nie ma mapera - za to jakiś handler na httpresponse, niby brzydkie, ale niczym się nie różni od klasyki (catch Exception e to też taki Any). Nic większego tak nie zrobiłem, ale mam wrażenie, że jest to kompromis, który można zastosować jeśli obsługa błędów zaczyna przyradzać się w static hyping.

Jeśli ify wynikają z logiki biznesowej (zlozonosc esencjonalna), to nie widzę w tym nic złego. Czasem lepszy jeden zagnieżdżony if niż jak wjeżdża turbo wzorzec strategii, który ma pod spodem jedna strategie ;p

Odnośnie tych monad i flatmapów - ja uważam to w 99% przypadków za przerost formy :) utrzymywałem taki system, gdzie były same mapy, flatmapy, combiney i było ciężko. Być może mam za niskie IQ i jestem słabym kompilatorem.