22
\$\begingroup\$

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?

\$\endgroup\$
  • 6
    \$\begingroup\$ Is there a reason you've preferred this over exceptions? \$\endgroup\$ – Ben Aaronson Sep 16 '14 at 16:25
  • 2
    \$\begingroup\$ We used error codes in a previous application and it quickly becomes a mess! \$\endgroup\$ – IEatBagels Sep 16 '14 at 17:28
  • 1
    \$\begingroup\$ Exceptions are for unpredictable errors, this case I can easily anticipate. Try-catch statements clutter the code, making it less readable. Exception handling tends to be expensive to handle flow control. \$\endgroup\$ – ThunderDev Sep 17 '14 at 9:07
  • \$\begingroup\$ @ThunderDev, there are certain things that you cannot do after receiving answers, Check out these Meta posts for more information, About answer-invalidating edits \$\endgroup\$ – Malachi Sep 17 '14 at 13:20
  • 1
    \$\begingroup\$ out is almost always a sign your function is doing too much. Return codes are as well. You should throw an exception if you have an issue with creating a new merchant. If the merchant already exists, then you should either be checking that the merchant exists based on predefined behaviour prior (merchant with the same name) - or you should allow the function to fail silently if one already exists. An exception is not used for flow control, it is used for an exceptional circumstance - a merchant already being created is an exceptional circumstance that the programmer should be aware of. \$\endgroup\$ – Dan Pantry Sep 17 '14 at 16:06
34
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ "The problem with a status code is that it can be ignored. An exception cannot." Could you expand on this? I don't see why it's harder to "ignore" an exception with a try-empty-catch than to discard the out parameter value. Either way the consumer knows about them \$\endgroup\$ – Ben Aaronson Sep 16 '14 at 17:09
  • 3
    \$\begingroup\$ The very reason you have to create a try-catch block tells other programmers that "something catastrophic happens here but I am blissfully ignorant of it". An empty try-catch is a code smell. \$\endgroup\$ – Greg Burghardt Sep 16 '14 at 17:14
  • \$\begingroup\$ Sure, I guess it's more obviously a smell/bad practice than ignoring an out paramter \$\endgroup\$ – Ben Aaronson Sep 16 '14 at 17:17
  • 4
    \$\begingroup\$ +1 for the empty catch block rant - I die a little inside everytime I see this, too! :) \$\endgroup\$ – Mathieu Guindon Sep 16 '14 at 18:02
  • 3
    \$\begingroup\$ It's not the execution of the method throwing an exception or returning a status code that needs to be stopped. It's the execution of the caller that needs to be stopped. This is the reason why exceptions work well. An exception is your way of telling the world at large that an insurmountable problem has occurred and someone must deal with it. Not dealing with it is not an option. In any field of science, failure is always an option. \$\endgroup\$ – Greg Burghardt Sep 17 '14 at 12:34
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This is only a small part of my code, it has many other steps, it's not limited to MerchantExists. My error codes are precisely there to define what to do in case a merchant exists or other cases. \$\endgroup\$ – ThunderDev Sep 17 '14 at 8:00
  • 4
    \$\begingroup\$ I have no idea why would anyone want to use ? true : false ternary operator :) \$\endgroup\$ – Nikita B Sep 17 '14 at 9:28
  • 1
    \$\begingroup\$ Just return _merchantRepo.GetMerchant(model.MerchantId) != null; is enough. \$\endgroup\$ – user11153 Sep 17 '14 at 11:59
  • \$\begingroup\$ That is the second time I have done that. Hammer says, "hey look another nail" \$\endgroup\$ – Malachi Sep 17 '14 at 13:25
  • 1
    \$\begingroup\$ @ThunderDev, it's so hard to tell with your code because you have only given a single method that has more responsibility than it should, and you left out the meat of it. from what I understand, yes, the controller should say "hey there is already a merchant so let's skip the creation and use the existing one" or "hey a merchant doesn't exist for this {insert object} let's create a new one for it" \$\endgroup\$ – Malachi Sep 17 '14 at 15:27
5
\$\begingroup\$

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 ;-)

\$\endgroup\$

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.