C# ASP.NET Is there an alternative to duplicating error checking on every controller action

user_4173159 0
Viewed 107

If I have a controller with quite a few actions, doing similar things e.g. Gets data from an api, does something with it, returns view with updated model. Is there a better way to handle errors. Currently I do this on a number of action methods, obviously this duplication doesn't feel right but I can't think of an alternative. Thanks

public async Task<IActionResult> method(string id)
{
  var result = await _flightRepository.GetLightById(id);
  if (!result.Valid)
  {
    return View("ErrorPage", result.Error.Message);
  }

  var viewModel = new FlightViewModel
  {
    Flight = result.Result
  };
  return View(viewModel);
}

Basically I want to somehow encapsulate the error handling logic to return the error view if valid is false, otherwise populate viewmodel and return view. The valid property returns true if there is an error with the request (this is done in api layer)

Thanks for any help

asp.net-core asp.net-mvc c#
2 Answers

If you use a static method like this:

static async Task<IActionResult> ProcessAsync<T>(
    Func<Task<RepositoryResult<T>>> process, 
    Func<T, IActionResult> ifValid
)
{
    var result = await process();
    if (!result.Valid)
    {
        return View("ErrorPage", result.Error.Message);
    }
    
    return ifValid(result.Result);
}

Then you can use it like this:

public Task<IActionResult> method(string id)
{
    return ProcessAsync(
        () => _flightRepository.GetLightById(id),
        flight =>
        {
            var viewModel = new FlightViewModel
            {
                Flight = flight
            };

            return View(viewModel);
        }
    );
}

The static method can be defined anywhere you wish - in a base Controller class, or in a different namespace entirely (in which case you would import it with using static Some.Namespace.With.Class;.

Additionally, I suggest you use the convention of an Async suffix for asynchronous methods - so because GetLightById returns Task<T> I suggest renaming it GetLightByIdAsync.

user_81993620

You should use Filters to reuse code among your actions.

For example in your case your filter could be:

public class FlightValidator: ActionFilterAttribute
{
    private readonly string _flightIdRouteKey; // e.g 23
    private readonly string _errorViewName; // e.g "ErrorPage"
    private readonly IFlightRepository _flightRepo;

    public FlightValidator(string flightIdRouteKey, string errorViewName, IFlightRepository flightRepository)
    {
        _flightIdRouteKey = flightIdRouteKey;
        _errorViewName = errorViewName;
        _flightRepo = flightRepository;
    }

    public override async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
    {
        int flightId = (int)context.RouteData.Values[_flightIdRouteKey];

        var result = await _flightRepo.GetFlightById(flightId);
        if (!result.Valid)
        {
            context.Result = new ViewResult
            {
                ViewName = _errorViewName, 
                ViewData = new ViewDataDictionary(result.ViewData)
                {
                    Model = model
                }
            };
            return;
        }

        await next();
    }
}

Now you can use the filter like this:

[TypeFilter(typeof(FlightValidator), Arguments = new object[] { "id", "ErrorPage"})]
public async Task<IActionResult> method(string id)
{
  var viewModel = new FlightViewModel
  {
    Flight = result.Result
  };

  return View(viewModel);
}

I suggest you to refer Filters docs for more information about filters.

user_60072440
Related