How to handle fails caused by business rules and return reason to end user?

Question

Let's say I have a asp.net-mvc application and I have to check that object can be deleted before deleting it.

public class ItemController : Controller
{
    public ActionResult DeleteItem(int itemId)
    {
        // Check if any customer currently uses this item.
        var customersUsingThisItem = _customerService.Get(customer => customer.SelectedItemId == itemId)
                                                     .Select(customer => customer.Name)
                                                     .ToList();
        if(customersUsingThisItem.Count != 0)
        {
            var message = $"This item is being used by {string.Join(",", customersUsingThisItem)}";
            // Return message to user...
        }

        // Delete item.
    }
}

Get method is defined as follows:

public IEnumerable<Customer> Get(Expression<Func<Customer, bool>> predicate)
{
    return context.Customers.Where(predicate);
}

It looks like this approach leaks business specific rules to a controller and the point of having services is to keep those rules inside services. Alternative approach would be to write a method:

public class ItemService
{
    public bool CanDeleteItem(int itemId)
    {
        // Check if item can be deleted.
    }
}

But now, how do I return reasons for why this item cannot be deleted?

I could add another parameter out string failReason, but this is not very meaningful and someone exposed to this method has no idea if this message will contain technical reasons or something that can be presented to end user.

Another option is to return enum with reasons such as:

public enum DeleteItemFailReasons
{
    None,
    ItemUsedInCustomers,
    // other reasons
}

but this does not tell which customers use this and with many methods I would end up with many many enums.

Throwing exceptions is also not good, because it would mean that I would have to used exceptions to follow program's flow, which is an anti-pattern.

Answer 1

One strategy can be that for a layer (like a service) that performs business rule validation, that you return a result from all your method calls that represents both a successful call, as well as a listing of any errors that prevents that call from running successfully.

public class Result<T> 
{
    public T ResultObject { get; set; }
    public List<ValidationResult> TransactionErrors { get; set; }
    public bool Success { get; set; }
}

public Result<Customer> GetNextCustomer(int i, string s) 
{
    // New up a Result<Customer>
    // Either attach a Customer to the Result or attach a listing ValidationResult errors,
    // and set Success=False
    // return that Result<Customer>
}

You need to be consistent in your approach here, so return Result or Result<T> for all calls out of that library, but I personally like returning objects from calls instead of throwing exceptions. You can still throw exceptions for EXCEPTIONAL things (couldn't connect to database, etc), and don't forget that the T in Result above can be anything, so you can attach partial successes or further custom objects as needed.

Answer 2

I would go with:

public List<ValidationResult> ValidateItemForDeletion(string itemId)

public class ValidationResult
{
    public bool Pass {get;set;}
    public string TestName {get;set;}
    public string FailReason {get;set;}
}

If you have a commonly occurring race condition add various LockItemForDeletion methods, to prevent testing and then failing to delete

Answer 3

Short summary

• Predicates give a lot of control, but possibly more than is necessary.
• Named tuples solve the issue of returning a boolean and a string nicely.
• Out parameters are inferior to named tuples.
• Enums are only usable internally, not as a return value.
• Exceptions are more acceptable than you suggest.
• (Not mentioned in the answer body) if you already have existing validation logic in other places of the codebase, it might be nicer to reuse the pattern as a matter of consistency.

---

public IEnumerable<Customer> Get(Expression<Func<Customer, bool>> predicate)
{
    return context.Customers.Where(predicate);
}

It looks like this approach leaks business specific rules to a controller and the point of having services is to keep those rules inside services.

It very much depends on what logic you're expecting. Some clear cut examples:

• Logic to hide soft-deleted customers should indeed not be the responsibility of the controller. It should be implemented in a repository layer.
• Logic to find all customers named "Bob" is part of the controller's duties, assuming that it's handling a request which contains a customer name filter.

Now, allowing a predicate to be passed this way gives your controller a lot of control. This has its upsides and downsides:

• Pro You don't spend time defining an intermediary layer where every filter/property/method is spelled out.
• Pro Less effort when refactoring queries.
• Con More effort when refactoring entities.
• Con Less separation of concerns.

This is a subjective decision. Different application have different needs and different focuses. If you have many different filters on an entity list, the cost of having to develop (and maintain) the intermediary layer increases.

You also run into issues such as having too many parameters in a method, or having to maintan method-specific classes to contain the parameters (which, in my opinion, is simply hiding the issue with having many parameters as opposed to fixing it).

---

public class ItemService
{
    public bool CanDeleteItem(int itemId)
    {
        // Check if item can be deleted.
    }
}

But now, how do I return reasons for why this item cannot be deleted?

Named tuples to the rescue here. I don't use them very often, because I'm apprehensive of being overly liberal with them.

However, this is one of those cases where the benefits vastly outweigh the potential for abuse:

public (bool reponse, string message) CanDeleteItem(int itemId)
{
    // Check if item can be deleted.
}

Its usage is rather elegant:

(bool canBeDeleted, string errorMessage) = ItemService.CanDeleteItem(123);

if(!canBeDeleted)
    MessageBox.Show(errorMessage);

Alternatively, if you're not a fan of named tuples, you can create your own Response method that has a bool and a string property. The principle is the same.

One thing to note though: this approach can become cumbersome if errors can occur on multiple levels. It requires constant wrapping, and every method might even need its own if wrapping to check the previous level's returned value.
This is actually where exceptions are more relevant to your case, because they are able to cross multiple levels without needing you to manually handle them every step of the way.

---

I could add another parameter out string failReason, but this is not very meaningful and someone exposed to this method has no idea if this message will contain technical reasons or something that can be presented to end user.

Since the latest improvements to tuples (syntactically simpler, named fields), I would argue that they are a better substitute for out parameters. out parameters were always a rather hacky way of returning multiple values.

It makes no syntactical sense to define an additional return value in the input parameters (out variable). It makes a lot more sense to defined an additional return value in the return type (as a named tuple).

I'm interested in hearing arguments against this claim, but I'm convinced that out parameters should be replaced with named tuples, except in cases of backwards compatibility with pre-existing libraries.

---

Another option is to return enum with reasons such as:

public enum DeleteItemFailReasons
{
    None,
    ItemUsedInCustomers,
    // other reasons
}

I like enums more than most of my coworkers, but I don't think this is a good reason to use them. It creates an additional maintenance task, for little or not benefit.
You're expecting the controller (or the requester) to be able to translate your enum into an error message, which means that the controller/requester must be aware of all possible errors. Furthermore, any changed/added error will require updates to the controller/requester, which means you're not segregating your responsibilities correctly.

Somewhat counterntuitively, if you were to store your error messages in a shared dictionary (as opposed to hardcoding them when you return an error), which can make sense if you use the same error message in multiple places, I would advocate using an enum as a key to that message dictionary.

But this second example (enum as a dictionary key) is different, as it remains internal to the business layer. Any changed/added errors would already require management (adding the message itself to the dictionary), so the added step of having to create an extra enum value does not (in my opinion) outweigh the benefit you get from intellisense autocompletion (which is the main reason why I advocate using the enum).

---

Throwing exceptions is also not good, because it would mean that I would have to used exceptions to follow program's flow, which is an anti-pattern.

Some of this is subjective. It's a matter of interpretation: is blocking behavior expected, or is it exceptional?

Shutting out the style argument for a moment, there is one objective difference: throwing and handling exceptions costs overhead. There is a performance cost attach to using exceptions.

This is the main reason why exceptions shouldn't be used wantonly to define program flow.

Like most things, it's not a zero-tolerance policy. It's a warning that when it is used ubiquitously, the performance hit will keep growing.
Similarly, stringFoo + stringBar + stringBaz isn't going to bog down your application. But if you scale this up and e.g. generate an entire CSV file like this, the performance hit will become an actual problem.

Doing it once is a negligble issue. The issue only becomes relevant as you repeatedly throw errors during a single request. The more you do it, the bigger the problem.

Exceptions are perfectly fine to use as an exit strategy. And that's what you're trying to do here: the business logic error is a blocking error, meaning that there will only be one exception thrown and then the request ends. You're not creating any logic that relies on repeated exceptions to define an algorithm's usual path.

You could, for example, create a BusinessException, so you can separate the catch logic. Business exceptions get shown to the user ad verbatim, whereas all other exception messages are used internally but not shown externally:

public string DeleteItem(int itemId)
{
    try
    {
        DeleteItem(itemId);

        return "Item deleted";
    }
    catch(BusinessException be)
    {
        return be.Message;
    }
    catch (Exception e) 
    {
        LogException(e);
        return "An unexpected error has occurred.";
    }       
}

Having this method return a string is maybe not the best approach here, but I'm just showing a simplified example of the catch logic here.