Returning status codes from business layer

Question

This is what I've been doing so far in my business layer to return error messages to the user.

public void CreateMerchant(MerchantCreationModel model, out MerchantCreationStatus status)
{
    //check if merchantId exists
    MerchantDTO merchant = _merchantRepo.GetMerchant(model.MerchantId);

    if (merchant != null)
    {
        status = MerchantCreationStatus.MerchantAlreadyExists;
        return;
    }

    // Rest of merchant creation logic has been removed for clarity

    status = MerchantCreationStatus.Success;
}

I have a ErrorCodeToString() method in my controller that gets the text to display.

What do you think of this technique?

Answer 1

Status codes have long gone the way of the Dodo --- made extinct by exceptions, actually.

The problem with a status code is that it can be ignored. An exception cannot. Consider these two examples.

Status Codes as Return Values

First, the "merchant repository" implementing CRUD operations for merchants:

public enum MerchantStatusCode
{
    OK = 0,
    Created = 1,
    Updated = 2,
    Deleted = 3,
    MerchantAlreadyExists = 4,
    MerchantDoesNotExist = 5
}

public class MerchantRepository
{

    public MerchantStatusCode Create(Merchant model)
    {

        if (MerchantExists(model))
            return MerchantStatusCode.MerchantAlreadyExists;

        // Save to DB

        return MerchantStatusCode.Created;
    }
}

Now, the C# code to create a merchant and ignore error handling:

var merchantRepository = new MerchantRepository();

// Create a merchant. I'm not really sure what could go wrong, and if
// something should go wrong, I'm not really sure if it's a problem.
merchantRepository.Create(merchant);

The MerchantRepository.Create method returns a status code that our programmer promptly ignores. By looking at these lines of code, there is no indication that you should do any error handling before subsequent lines of code are executed.

Status Codes as Output Parameters

Let's also say an output parameter is used:

public class MerchantRepository
{
    public void Create(Merchant model, out MerchantStatusCode statusCode)
    {

        if (MerchantExists(model))
        {
            statusCode = MerchantStatusCode.MerchantAlreadyExists;
            return;
        }

        // Save to DB

        statusCode = MerchantStatusCode.Created;
    }
}

And the code to ignore the status code:

var merchantRepository = new MerchantRepository();
MerchantStatusCode code;

// Create a merchant. Sure I could check the "status" but, meh. If
// something fails it must not be THAT important. After all, good
// programmers are lazy, right?
merchantRepository.Create(merchant, out code);

While this exhibits more of a code smell, an error code doesn't halt program execution.

Now, let's tweak this to throw an exception:

Using Exceptions

public class MerchantRepository
{
    public void Create(Merchant model)
    {
        if (MerchantExists(model))
            throw new RecordExistsException();

        // Save to DB
    }
}

public class RecordExistsException : Exception
{
    ...
}

And the C# code to use this in the same manner as the previous example where the programmer blissfully ignores problems:

var merchantRepository = new MerchantRepository();

try
{
    merchantRepository.Create(merchant);
}
catch Exception
{
    // I realize this method throws exceptions when it fails to create
    // a merchant, but I am a complete idiot as a programmer, and
    // failure as a human being, so I'm going to blindly swallow any
    // possible errors, then move to another team or job and let you
    // try tracking down this production problem for 6 months while
    // the company bleeds customers and your boss screams at you so
    // hard he spits his bagel and coffee all over your face.
    //
    // But, hey. At least I didn't write "TODO: Error handling"!
}

Both examples work the same when the code is executed and a duplicate merchant is created, but there is one glaring difference here: The empty try-catch block. Empty try-catch blocks go beyond "code smells" to "steaming piles of rotting flesh that have been baking in the sun all afternoon." I get an ulcer every time I see this.

Now, if we tweak example #2 to be the exact same code as example #1:

var merchantRepository = new MerchantRepository();

// Even though I don't handle an exception, at least the next
// line of code won't execute if something goes terribly wrong.
// I'm not an idiot. I just HAVE to create a merchant here, and
// failure to do so is a hard stop.
merchantRepository.Create(merchant);

No subsequent lines of code will get executed, and the programmer does not have to guard against this condition unless they want to.

The lesson here: For the love of God do not blindly trap exceptions and do nothing, unless you document this behavior in comments and provide a really really really really really really really really really really really really really good reason for doing so.

Regarding your particular approach, there are a couple things that you should be doing instead:

Error Handling Best Practices Regarding Database Operations

1. Prior to creating a merchant, check the database to ensure the merchant doesn't already exist. This is basic data validation, and should be handled in its own layer.
2. When creating a merchant, if a merchant happens to exist, throw an exception which will force you, as a programmer, to handle this "exceptional situation" (hence the term Exception).

When a database operation is invoked, it needs to assume that all data is valid, and then throw an exception if anything goes wrong. Things should never be wrong in the first place, and the data access layer has no way of remedying this problem, so throwing an exception halts any further operation. Failing to create a merchant is a catastrophic, unresolvable issue requiring the immediate cessation of logic.

If attempting to create a duplicate merchant is something you can handle, then your Controller needs to trap this exception and handle it.

An interesting blog post about error codes and exceptions:

Error codes or Exceptions? Why is Reliable Software so Hard?

There can be a case for either, but I generally lean towards exceptions because they halt application execution, which is what you want most times.

Answer 2

something is wrong here.

public void CreateMerchant(MerchantCreationModel model, out MerchantCreationStatus status)
{
    //check if merchantId exists
    MerchantDTO merchant = _merchantRepo.GetMerchant(model.MerchantId);

    if (merchant != null)
    {
        status = MerchantCreationStatus.MerchantAlreadyExists;
        return;
    }

    // rest of merchant creation logic

    status = MerchantCreationStatus.Success;
}

This isn't a CreateMerchant Method.

This is a badly written MerchantExists Method

I would make this a boolean method and do something like this

public boolean MerchantExists(MerchantCreationModel model)
{
    if (_merchantRepo.GetMerchant(model.MerchantId) != null)
    {
        return true;
    }
    return false;
}

Or if you like Ternary statements like me you could write it like this

public boolean MerchantExists(MerchantCreationModel model)
{
    return _merchantRepo.GetMerchant(model.MerchantId) != null ? true : false;
}

So this is not the first time I have made made something a ternary that obviously didn't need to be one, this code is way more simply written like this

public boolean MerchantExists(MerchantCreationModel model)
{
    return _merchantRepo.GetMerchant(model.MerchantId) != null;
}

This will return a true or false value depending on whether the expression is true or false...

Hammer says,

"Hey look, a funny looking nail. I can fix it."

---

I really think that your _merchantRepo object's class should have it's own MerchantExists Method so that you could just say

if (_merchantRepo.Exist(MerchantID))
{
    //Perform Tasks on Merchant... etc.
}
else
{
    //Create Merchant
}

This lends a hand to checking this in your Data Layer like @Greg Burghardt hints towards in his answer.

I just don't think that you actually need Error Handling here, you just need to define what to do if the merchant doesn't exist, so make a way that the MerchantRepo can notify others that the merchant doesn't exist, yet, and then create a new one.

Answer 3

I personally agree with Exceptions over Status Codes principle.

However, once when having really deep and scattered business logic (with pluginable strategies etc.) we decided to mix them together. We created some special BusinessResult class (and BusinessResult<T>), that was able to contain everything: result itself, exception if happened, status codes etc.

Some layers used to throw exceptions, that some of them we were able to catch and convert into common status codes on various execution depths. If one tried to use Value of the BusinessResult<T> that was in Failed state - then one would receive an InvalidOperationException - so one was not able to ignore StatusCode easily.

Then - according to received status severity - we were able to: decide to try some operations again or to rollback whole transactions and then try again, or even to shut down whole services or only set it to idle mode. We had some overloaded constructors, implicit and explicit operators that allowed us to convert common error situations into particular Result with ease and clean code.

Now, when I look on that from the perspective I see that this approach was anyway very complicated and required us to care both for exceptions and status codes - it was quite fine in our main business module that was supposed to marry several different subsystems into one flow - but was quite overwhelming to use in simple scenarios.

I do not have access to that code now to show some examples, but that's fine - if one is bold enough to use it, one has to also be tough enough to create it on his own ;-)