2

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.

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.

0

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

1
  • Hah, you reminded me that in my project I already have custom validator class and ValidationRule class, but I use it only in controllers, not services. – FCin Jun 26 '18 at 9:47
0

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.

4
  • 1
    I have to respectfully disagree with your last section. Using exception handling for workflow is never acceptable. Not because of performance issues but rather because it obfuscates real, legitimate errors. As someone who has had to maintain and refactor legacy code for a team that did not understand this concept, I can attest that this practice does indeed create spaghetti code. Personally, I'd rather see a developer use a goto statement. At least there you can use your IDE's find functionality to easily identify what code needs to be changed. – DanK Jul 26 '18 at 13:23
  • @DanK: Personally, I'd rather see a developer use a goto statement. I don't quite follow. Goto statements and exception bubbling are two very different things. Keep in mind that in the last example, DeleteItem(itemId) can have several layers worth of code, each of which could throw an exception (intentional or not) at any time; including precompiled libraries where you do not have access to the code to implement a goto. I don't see a way for a precompiled library to use a goto statement that points towards your particular code, let alone if you use the same library multiple times. – Flater Jul 26 '18 at 13:36
  • 1
    Using exception handling for workflow is using a goto statement. You are simply using the language internals to jump over code rather than explicitly creating the statements yourself. With exception bubbling, Microsoft has provided sugar that allows a developer to conveniently pass an exception to the UI. But this functionality is meant to be used only for exceptions, not end user messaging. I'd recommend this for a more articulate explanation. – DanK Jul 26 '18 at 16:16
  • I would add my 2 cents in @DanK comment. When you know something will result in an error, it is no more an "exception". Better approach would be to return a structure like publc class ErrorResult { ContainsError (bool), ErrorId (int), Message (string) } and reuse this in all methods – adeel41 Jul 27 '18 at 13:20

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.