Obsługa błędów za pomocą Either, funkcyjne przykozaczenie - czy robię to dobrze? | Mały code review

0

Zaczynam nowy projekt. Na razie napisałem kilka klas, które będą podstawą działania systemu. Do obsługi błędów używam Either z Vavra (spodobał mi się ten pomysł, piszę tak pierwszy raz).

Na tą chwilę na pewno trzeba tu jeszcze ulepszyć obsługę błędów - i tu nie do końca mam pewność, jak to najlepiej zrobić.
Aktualnie zawsze zwracam błąd jako Stringa. Zazwyczaj tak:

Either<String, String> foo() {
    try {
        //
    } catch (Exception e) {
        e.printStackTrace();
        return Either.left(e.getClass().getSimpleName() + " " + e.getMessage());
    }
}

i tu wiem, że muszę zmienić zawartość błędu na coś bardziej opisowego. Byłoby fajnie zwrócić treść błędu, która nadaje się do wyświetlania użytkownikowi. Ale chcę też widzieć co dokładnie poszło nie tak. Może na potrzeby lewego Eithera stworzyć taką klasę:

class Error {
    String userFriendlyMessage; //info dla użytkownika
    Exception exception;  //info dla mnie
}

i w kontrolerach zwracać użytkownikowi userFriendlyMessage?

Staram się pisać czysto. Zapraszam do krytyki :)
https://github.com/Potat0x/PotaPaaS-Service/tree/master/src/main/java/pl/potat0x/potapaas/potapaasservice/util
https://github.com/Potat0x/PotaPaaS-Service/tree/master/src/test/java/pl/potat0x/potapaas/potapaasservice/util

5

Jak już chcesz iść w vavra to po całości. Zobacz jak działa Try i zacznij używać list z vavra ;)

Co do tego co chcesz zwracać to musisz pomyśleć jak Ci wygodnie i co potem chcesz z tym zrobić. U mnie jest to enum które potem mapuje na odpowiednią odpowiedz http

Poza tym

Thread.sleep(1000); w teście wygląda średnio ;)
I nazwa pakietu util niewiele mówi

Zamiast

GitCloner(String targetPath) throws Exception {
        if (new File(targetPath).isDirectory()) {
            this.targetPath = targetPath;
        } else {
            throw new Exception(targetPath + " is not directory");
        }
    }

Możesz zrobić statyczną metodę która zwraca Either z tworzonym obiektem albo info dlaczego sie nie udało

To tak na razie na szybko

1

Ja przeważnie tak stosuje eithera

  public Either<Exception, User> getUserByLogin(String login) {
    return userRepository.findByLogin(login).map(Either::<Exception, User>right)
        .orElse(Either.left(new UserNotExist()));
  }

0

@danek
W kilku miejscach użyłem Try.of i Try.run. Kod jest wtedy bardzo fajny, nierozwlekły, ale jest to przepychanie wyjątku przez system, a trzeba go gdzieś obsłużyć - i chcę to zrobić jak najszybciej (oczywiście, jeżeli ma to sens).
Pewnie zdecyduję się na Eithery i jakiś typ na przechowywanie błędów (Twoje rozwiązanie z enumem bardzo mi się podoba). Czyli w najniżej położonych funkcjach taki kod (albo jego ładniej wyglądający odpowiednik):

    try {
        return Either.right(...);
    } catch (Exception e) {
        e.printStackTrace();
        return Either.left(Match(e).of(...)); 
    }

Dodatkowa korzyść to e.printStackTrace(); i mapowanie wyjątków na komunikaty błędów bezpośrednio na miejscu zdarzenia.

Thread.sleep(1000); w teście wygląda średnio ;)

Wiem, że to nie jest idealne rozwiązanie, ale jest jakieś inne wyjście (bez wielu komplikacji)? Muszę poczekać, aż kontener wystartuje.

I nazwa pakietu util niewiele mówi

Racja, zmieniłem na core.

Możesz zrobić statyczną metodę która zwraca Either z tworzonym obiektem albo info dlaczego sie nie udało

Zrobione.

@krancki
Zależy mi na szybkim obsłużeniu wyjątków, żeby zaciemniać kodu obsługą błędów ;)

2

Dodatkowa korzyść to e.printStackTrace();

A to jest korzyść? ;)

Jeśli piszesz swój kod, to nie rzucaj wyjątków w ogóle, jeśli używasz coś co może rzucić wyjątkiem, to opakuj w Try. Try możesz na końcu łatwo przemapować na odpowiedni Either łatwo ;)

Co do sleep
Hm, pytanie zasadnicze czy zawsze chcesz czekać w teście aż wystartuje. Ja bym podzielił testy na dwie grupy. Te, które odpalasz często i tam zrobił bym jakieś mocka/stuba/fake który tylko by udawał start aplikacji i działał od razu i drugą grupę testów, rzadziej odpalanych które testują już faktycznie wszystko.

0

@danek a jeżeli ustawiamy exceptionHandler w springu ? Przykład niżej.

@GetMapping(value = "/{userId}", produces = "application/json")
  public ResponseEntity<UserDTO> getUser(@PathVariable("userId") final long id) {
    return userService.getUser(id).map(user -> ResponseEntity.status(HttpStatus.OK).body(user)).getOrElseThrow(e -> {
      throw e;
    });
  }

i gdzieś tam tam

@RestControllerAdvice
public class GlobalExceptionHandler {

  @ResponseStatus(HttpStatus.CONFLICT)
  @ExceptionHandler(RuntimeException.class)
  public ResponseEntity<String> handleConflict(RuntimeException ex) {
    return ResponseEntity.status(HttpStatus.CONFLICT).body(ex.toString());
  }
}

1

@krancki: Either jest właśnie po to aby pisać w inny sposób, bez magicznego wychodzenia z metody za pomocą takiego "goto".

0

A to jest korzyść? ;)

Widać dokładnie co i gdzie się wysypało.

Co do sleep
Hm, pytanie zasadnicze czy zawsze chcesz czekać w teście aż wystartuje. Ja bym podzielił testy na dwie grupy. Te, które odpalasz często i tam zrobił bym jakieś mocka/stuba/fake który tylko by udawał start aplikacji i działał od razu i drugą grupę testów, rzadziej odpalanych które testują już faktycznie wszystko.

Celem tego testu jest sprawdzenie, czy aplikacja wystartowała, więc nie ma jak tego zamockować :P Ale dalej mockowanie może się sprawdzić, w testach dla wysokopoziomowych klas - jeszcze wyjdzie w praniu, szczególnie że teraz wszystkie testy trwają około minutę. Klonowanie z GitHuba, budowanie obrazów, stawianie kontenerów, trochę to trwa :P Chociaż problem klonowania można rozwiązać, stawiając na potrzeby testów Gita w kontenerze.

2

A po co chcesz sprawdzać czy inne narzędzia działają? ;) Testy mają sprawdzać czy logika którą Ty napisałeś działa

0

Narzędzia działają, ale mogłem im podać zły parametr (np skopane przemapowanie portów kontenera) - co jest błędem w napisanej przeze mnie logice. Wydaje mi się, że przypadek jest nietypowy :P

1

hardcodowanie paramterów to raczej słaby pomysł. Lepiej jakbyś je pobierał skądś niż miał w kodzie. No i podanie złych parametrów wejsciowych nie jest błędem w logice tylko w podaniu złych parametrów wejsciowych ;)

Imo powinieneś zrobić tak, że apka bierze skądś te parametry, wczytuje, odpala co ma. Wtedy możesz też obsłużyć przypadek kiedy faktycznie ktoś poda złe (jakoś wykrywać błędy). Wtedy w testach faktycznie testujesz tylko tylko sam fakt odpalenia i ewentualne szukanie błędów. Dzięki temu możesz się uniezależnić od czekania aż obraz w dokerze faktycznie wstanie tylko mockujesz/fakeujesz (kek ta odmiana) jego zachowanie.

0

Nawet nie chodzi o hardkodowanie. Jeżeli testy tej metody przejdą to wiem, że w builderze nie zapomniałem żadnego parametru, że buildType ma wartość RELEASE, gdy tego oczekiwałem. Użytkownik nie podaje tych parametrów, nie są to parametry wejściowe. Są ustalone lub generowane przez inną część systemu - w 100% ja za nie odpowiadam. I widać też, że mam tu jeden parametr zahardkodowany - przeniosę to do pliku.

Myślę, że na mocki jeszcze przyjdzie czas, jak zacznę klepać kontrolery Springa i takie tam. Na razie nie widzę jak albo nie ogarniam :P

PS: w sumie trochę namieszałem w poprzednim poście - gdy mówiłem o parametrze miałem na myśli argumenty dla innych narzędzi (np. Docker) przekazywane przeze mnie w kodzie, a nie takie typowe parametry wejściowe aplikacji.

2

Jeżeli testy tej metody przejdą to wiem, że w builderze nie zapomniałem żadnego parametru

Coś tu brzmi źle. Jeśli jakieś parametry są obowiązkowe to nie powinny zależeć od tego czy ich nie zapomnisz ;)

0

Jedne są obowiązkowe - wtedy test natychmiast się wywala, a inne takie, że kontener może zostać uruchomiony, ale nie wystawi aplikacji na świat (jeżeli zapomnę ustawić mapowanie portów, albo coś źle zrobię), co test też wykryje.
Wynika to z tego, że builder nie wymusza podania parametru. Kiedyś przy pisaniu buildera użyłem refleksji do wymuszenia parametrów :D Niestety efekt (rzut wyjątkiem) jest dopiero po skompilowaniu i uruchomieniu aplikacji.

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