Rzucanie wyjątkami w serwisach - jest sens?

Powiedzmy, że w kontrolerze wywołuję metodę serwisu, która może rzucić 4 różnymi wyjątkami. Do każdego z wyjątków piszę jakiś ExceptionHandler. Nagle okazuje się, że metoda kontrolera jest oznaczona 8 atrybutami! Czy to zgodne z MVC, gdy logika prezentacji zostaje usunięta z kontrolera i przeniesiona do jakichś handlerów, a metody kontrolera udają, że nic złego nie może się stać? Z jednej strony jest to wygodne (mniejsza duplikacja kodu), ale z drugiej strony mam nieustannie wrażenie, że tak nie powinno być. Od C# 7 funkcje mogą zwracać kilka wartości. Czy nie powinno się tego wykorzystać do obsługi błędów? Albo może lepiej by było napisać jakiś generyczny ActionResult (wiem, nazwa niefortunna) i zwracać go z metod serwisów?

Nie rozumiem.

Jak to logika prezentacji w handlerach.? Jakich handlerach?

Nie musisz pisać do każdego exception innego handlera w sumie to jest to bez sensu.

Wydaje mi się, że nie rozumiesz czym się różni wyjątek od walidacji lub stanu aplikacji, no chociaż wyjątek to też stan tylko, że wyjątkowy. ;#

Ja bym jednak był za wyjątkami, zwracanie wartości "poprawności" wykonania metody poprzez tuple jest tak samo głupie jak zwracanie przez funkcję która ma zwrócić listę czy inny typ referencyjny null'a.

Jeżeli chodzi o MVC bardzo często korzystam w kontrolerach z DI i nie boję się tego że coś mi rzuci wyjątek, dobra obsługa wyjątków to podstawa.

Nie rozumiem też po co Ci ExceptionHandler co chcesz nim osiągnąć? chcesz wiedzieć że wystąpił wyjątek? użyj logger'a.

Źle się wyraziłem, chodziło mi o ExceptionFilters. Na SO piszą, że ExceptionHandler powinien być jeden w całej aplikacji i służyć do obsługi wyjątków, które nie zostały obsłużone przez ExceptionFilters. Te z kolei obsługują określone rodzaje wyjątków (np. zwracając odpowiedni widok w odpowiedzi). Nie wiedziałem o takim podziale.

No to teraz wiem o co Ci chodzi, zerknij jeżeli nie widziałeś:

https://docs.microsoft.com/en-us/aspnet/web-api/overview/error-handling/exception-handling

https://docs.microsoft.com/en-us/aspnet/web-api/overview/error-handling/web-api-global-error-handling

nobody01 napisał(a):

Czy to zgodne z MVC, gdy logika prezentacji zostaje usunięta z kontrolera i przeniesiona do jakichś handlerów, a metody kontrolera udają, że nic złego nie może się stać?

Handlery czy różne filtry, to po prostu sposób uniknięcia duplikacji kodu w warstwie prezentacji, kontrolery przez to niczego nie udają.

Albo może lepiej by było napisać jakiś generyczny ActionResult (wiem, nazwa niefortunna) i zwracać go z metod serwisów?

Jak najbardziej należy tak zrobić. Wyjątki nie powinny być używane w sytuacjach, które wyjątkowe nie są.

Deltech napisał(a):

Ja bym jednak był za wyjątkami, zwracanie wartości "poprawności" wykonania metody poprzez tuple jest tak samo głupie jak zwracanie przez funkcję która ma zwrócić listę czy inny typ referencyjny null'a.

A przepływ sterowany wyjątkami, to niby jest mądry?!

Nagle okazuje się, że metoda kontrolera jest oznaczona 8 atrybutami!

Chyba nie, aż tak nagle... W Go jedną ze zwracanych wartości może być wartość błędu, ale z tego co wiem w C# tak się nie robi. W C# wyjątek jest rzucany (throw) i odpowiednio obsługiwany w konstrukcji try-catch. Mnie uczono, że dobrą praktyką jest nie tylko rzucić wyjątkiem gdy jest to potrzebne, ale także odpowiednio je logować co bardzo pomaga przy debugowaniu aplikacji.

@siloam: Nie programuje w C# ale powinna być chyba jakas biblioteka która ma Either i inne tego typu typy które pomagają. W Javce jest VAVR

@somekind: Taki kod byłby OK?

public async Task<ActionResult> Edit(EditQuestionViewModel model, int id)
        {
            var result = await questionStore.Edit(model, id, User);

            if (!result.ActionSucceeded)
            {
                ViewBag.ErrorMessage = result.Error.Description;

                switch (result.Error.Name)
                {
                    case ErrorName.NotFound:
                        return new HttpNotFoundResult();
                    case ErrorName.NotAuthorized:
                        return new HttpUnauthorizedResult();
                }
            }

            return RedirectToAction("Details", new { id = id });
        }

public class Result<T>
    {
        public readonly T Value;

        public readonly Error Error;

        public readonly bool ActionSucceeded;

        public Result(T value, Error error = null)
        {
            Value = value;
            Error = error;

            if (error == null)
            {
                ActionSucceeded = true;
            }
        }
    }

    public static class ResultFactory
    {
        public static Result<T> Success<T>(T value)
        {
            return new Result<T>(value);
        }
        
        public static Result<T> Error<T>(ErrorName errorName, string errorDescription)
        {
            var error = new Error(errorName, errorDescription);

            return new Result<T>(default(T), error);
        }
    }

    public class Error
    {
        public readonly ErrorName Name;

        public readonly string Description;

        public Error(ErrorName name, string description)
        {
            Name = name;
            Description = description;
        }
    }

    public enum ErrorName
    {
        NotFound,
        NotValid,
        NotAuthorized,
        ...
    }

Ja bym zamienił ten ResultFactory na statyczne metody w klasie Result.

1. Przede wszystkim to C#, więc wypadałoby używać właściwości, a nie pół public readonly.
2. Gdybyś w klasie Result<T> dwa konstruktory - jeden dla T, a drugi dla Error, to byłoby o wiele czytelniej i nie miałbyś takich dziwnych konstrukcji: return new Result<T>(default(T), error);
3. ResultFactory to tylko wrapper na konstruktor, w moim odczuciu całkowicie zbędny.
4. Lepiej byłoby mieć public bool ActionSucceeded => Error == null; niż pole ustawiane w konstruktorze.
5. Autoryzację to lepiej jednak robić przez Authorize albo jakiś swój filtr wykonywany przed akcją.
6. Przy tym podejściu if (!result.ActionSucceeded) będziesz miał w wielu miejscach, trzeba wymyślić, jak tego uniknąć.

Ojej, nie mam pojęcia, o czym myślałem, pisząc ResultFactory. Chyba o niczym :( A to sprawdzanie, czy akcja się powiodła, realizować przy użyciu jakiejś klasy typu ControllerHelper, której odpowiednią metodę potem wywoływać po ifie, czy może jakoś inaczej? A co do tego NotAuthorized, to w serwisie sprawdzam, czy użytkownik może w ogóle edytować poprzez porównanie id autora i użytkownika. Powinno się taką walidację umieszczać gdzieś indziej?

W dobrym kierunku idę?

public class BaseController : Controller
    {
        private ActionResult HandleNotFoundError(Error error)
        {
            return new HttpNotFoundResult();
        }

        private ActionResult HandleNotKnownError()
        {
            return View("Error");
        }

        private ActionResult HandleError(Error error)
        {
            ViewBag.ErrorMessage = error.Description;

            switch (error.Name)
            {
                case ErrorName.NotFound:
                    return HandleNotFoundError(error);
                // ...
                default: return HandleNotKnownError();
            }
        }

        protected ActionResult CreateResponse<T>(Result<T> result, Func<ActionResult> actionSucceeded)
        {
            if (!result.Succeeded)
                return HandleError(result.Error);

            return actionSucceeded();
        }

        [Route("questions/{id}/edit")]
        [HttpPost]
        [ValidateAntiForgeryToken]
        [ValidateModelState]
        public ActionResult Edit(EditQuestionViewModel model, int id)
        {
            var result = questionStore.Edit(model, id, User.Identity.GetUserId());

            return CreateResponse(result, () => RedirectToAction("Details", new { id = id }));
        }

Jak dla mnie spoko.