2
\$\begingroup\$

I'm making my WebApi project.

My logic is, the controller didn't see data. It only triggers a service that returns objects to it. My simple method from TaskService looks like this:

public Task TakeTaskByUser(int taskId, string userId)
{
    var task = this.GetItem(taskId);

    if (task != null)
    {
        if (task.ApplicationUserId != null)
        {
            throw new TaskTakenByAnotherUserException();
        }

        task.ApplicationUserId = userId;
        task.StartTime = DateTime.Now;

        this.context.Entry(task).State = EntityState.Modified;
        this.context.SaveChanges();

        return task;
    }

    return null;
}

As you can see, in this method I try to find Task and attach Foreign Key to User by his UserId. If Task is already taken by another user, I throw a custom exception.

In my controller, Action looks like this:

[HttpPut("{taskId}/{userId}")]
[Authorize(Roles = "Developer, Manager")]
public IActionResult TakeTaskByUser([FromRoute] int taskId, [FromRoute] string userId)
{
    try
    {
        var task = this.taskService.TakeTaskByUser(taskId, userId.ToString());
        if (task != null)
        {
            return this.Ok(task);
        }

        return this.NotFound();
    }
    catch (TaskTakenByAnotherUserException)
    {
        return this.ValidationProblem();
    }
}

Is using a try-catch block in a controller good practice? Is using any exceptions in WebApi good, or I should use the method posted in this article (I like Level 1, seems easy and fun)?

How do I deal with errors that I know that can appear when someone sends a request?

\$\endgroup\$
3
\$\begingroup\$

Exceptions are pretty costly so as a general rule of thumb they should be avoided. Especially when we talk about REST endpoint where response time is crucial.

Also, to my taste you're messing up command-query separation principle in your code. I would have rewritten it roughly like this

[HttpPut("{taskId}/{userId}")]
[Authorize(Roles = "Developer, Manager")]
public IActionResult TakeTaskByUser([FromRoute] int taskId, [FromRoute] string userId)
{
    var task = this.taskService.GetTask(taskId);
    if (task == null)
        return this.NotFound();
    var taskValidationResult = this.taskValidator.Validate(task);
    if (!taskValidationResult.IsSuccess)
    {
        //handle validation failure
    }

    task = this.taskService.TakeTaskByUser(task, userId);

    return this.Ok(task);
}
```
\$\endgroup\$
  • \$\begingroup\$ I like that method to avoid exceptions. I will try it, thanks! \$\endgroup\$ – michasaucer Jun 5 at 10:21
  • \$\begingroup\$ @Bohdan Stupak how would you handle validation failures? Ignore silently, wrap as some kind of response or perhaps throw an exception ? \$\endgroup\$ – dfhwze Jun 5 at 11:59
  • \$\begingroup\$ I would return some kind of response to be consistent with return this.NotFound() couple lines above. I've omitted the code because the mapping of validation result to response code might be verbose and irrelevant for the case. \$\endgroup\$ – Bohdan Stupak Jun 5 at 12:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.