22
\$\begingroup\$

I have recently been refactoring some code in an effort to improve the exception handling, particularly to aid in improving the level of information given to developers during development. However I am worried that I have started down the wrong path.

For example we were getting complaints that the following code threw an exception with the message: Sequence contains no elements when a database does not contain a record with the supplied ID.

public Record GetRecord(string id)
{
    _dbContext.Records.First(r => r.Id == id);
}

After refactoring the code, it now throws an exception whose message is No record found with id: {0}.

public Record GetRecord(string id)
{
    try
    {
        _dbContext.Records.First(r => r.Id == id);
    }
    catch(InvalidOperationException ex)
    {
        throw new ApplicationException(string.Format("No record found with id: {0}". id), ex);
    }
}

I can understand that this error message might be clearer, as we now know that no record was found. However, using this logic, where should we stop? Surely the below would also be an 'improvement'

public Record GetRecord(string id)
{
    try
    {
        _dbContext.Records.First(r => r.Id == id);
    }
    catch(InvalidOperationException ex)
    {
        throw new ApplicationException(string.Format("No record found with id: {0}". 
    }
    catch(SqlException ex)
    {
        throw new ApplicationException("An error occurred while processing the record"), ex);
    }
}

I would consider it bad practice to wrap exceptions like this, however I am willing to be proved wrong. Is there any reason why this would be desirable? Where should the line be drawn?

Microsoft provide the following advice

Consider wrapping specific exceptions thrown from a lower layer in a more appropriate exception, if the lower layer exception does not make sense in the context of the higher-layer operation.

This should be done rarely because it makes debugging more difficult. It is appropriate when you are certain that the lower layer is never the true source of the error.

However surely an exception can also be wrapped in a more appropriate exception?

\$\endgroup\$

closed as off-topic by 200_success Mar 1 '18 at 0:21

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

20
\$\begingroup\$

There is little reason to do this. If you include the stacktrace in your logs you already know where the problem exists; the only thing you'd miss is the input which you might know from different channels (and often isn't necessary to figure out the problem).

Noting that your example returns void which is probably not what you intended, I should say that the change I would make is to use .FirstOrDefault() instead of First which will instead return the default value - null for a reference type. You can then work with a simple null check to see whether or not a record was returned instead of having to start using try-catches.

Yes, you could do all this to provide specific error messages. Personally I am not a fan of it and I prefer to derive this from the code itself (through for example the before-mentioned null check).

\$\endgroup\$
  • 2
    \$\begingroup\$ @SaulMarquez If you do an lookup-by-ID and the record is not there, it is an exceptional case. Either the code is wrong (always a possibility), someone deleted the record (System.Data.DataException??), or someone is doing request forgery. I cannot think of an ordinary course of events that will lead to lookup-by-ID to fail. \$\endgroup\$ – abuzittin gillifirca Oct 17 '14 at 13:30
  • \$\begingroup\$ @abuzittingillifirca: one example is looking up something by ID that has been since deleted. You might have the id around somewhere still and make a search on it. \$\endgroup\$ – Chris Oct 17 '14 at 13:48
  • \$\begingroup\$ @Chris which IS an exceptional case. Unless you have code to deal SPECIFICALLY with deleted rows, AND you are sure its a deleted row, it is better to allow the exception to bubble up. \$\endgroup\$ – Aron Oct 20 '14 at 17:27
  • \$\begingroup\$ @Aron: It can be but at the same time I don't think it has to be. If you are allowing deleting then it seems quite reasonable to me that you might end up looking for something that has been deleted and thus should handle this. Its all a matter of design choices but hopefully you can see that there are times that a non-existant id is predictable behaviour. \$\endgroup\$ – Chris Oct 20 '14 at 17:31
  • \$\begingroup\$ I disagree. There is one big reason to do this if you are dealing with vendor libraries. For example, the only kind of exception Oracle ever throws is OracleException; client has to figure out the error by looking up error number. By writing a single switch once I can easily distinguish between "inseufficient permissions" and "invalid request" situation easily and simply rethrow the exception without clients having to know about Oracle. Basically, I can have same range of exceptions across all of backend systems (be it database, web-service, etc.) \$\endgroup\$ – DarkWanderer Feb 3 '15 at 13:20
14
\$\begingroup\$

It depends on how you wish to present the error state to your end user.

A rule of thumb: Always log the original exception (which is lower-level and very often quite technical and would scare nontechnical users). But wrap the exceptions in more user-friendly exceptions (higher-level) with messages that are in plain language and easy to understand for the users.

You don't need to (and can't) do this for every possible exception but generally you should do this for error states that can reasonably be expected to occur. In your example, if InvalidOperationException can be generated when and only when a record with the user-entered record ID does not exist, then it's reasonable to wrap it in a friendly message like "A record with the ID xyz could not be found.".

Likewise, SqlException can be expected when there is any sort of problem connecting to the database, and unless your user base is technical and understands SQL, databases, etc it is better to wrap them in higher level exceptions with friendly messages like "Something went wrong. Please check your network connection. If the problem occurs again, please contact your system administrator or software vendor.".

Depending on how your application displays the error state to the user, a "More Details" view can display the original exception and full stack trace (which is attached as cause in the wrapped exception) but such a view should be hidden by default. This has some advantages. An on-site support person can see the original exception immediately without scouring the logs; and if the user happens to be technical (in a generally nontechnical user base), the user can also better describe the error to over-the-phone support.

Also, avoid hard-coded strings (if you aren't already avoiding them). It makes them non-reusable and non-localizable.


A note about checking for data validity:

Abbas recommends that exceptions should only be allowed to be generated for unexpected behavior, and proceeds to give an alternative on how to check for the existence of a record (as the fact that a record may or may not exist is expected, not unexpected) without the use of exceptions. While this is generally desirable and works well for simple checks, I would recommend against it for more complex checks such as trying to parse an user-entered string into a number.

A user-entered string can be expected to not represent a number, and parsing it to number can fail. You may be tempted to write regex checks to perform on the string before parsing, but not only are you trying to replicate what the language already provides, you may miss out edge cases and still get the parse exception anyway - and that should not crash your application entirely. In this case, it's better to blindly attempt the parsing using the language's standard parsing methods and let an exception be generated, then handle the exception by alerting the user that the entered string is not a valid number.

However, if the language's standard parsing library provides methods to test the parse and fail gracefully, you can use them instead of testing the parse yourself and catching exceptions. As Abbas notes in comment, there is a TryParse method in C# which functions similarly to the TryGetRecord method proposed in Abbas' answer. Similar implementations don't exist in Java (mainly due to lack of out parameters), where catching exceptions is the right way to go.

\$\endgroup\$
  • \$\begingroup\$ I agree that in some cases an exception is necessary but when parsing user input I (and Microsoft too I guess) prefer not to throw expcetions. An example is the Int32.TryParse method, this method uses the same technique as I did with the out parameter. This way, a faulty input is caught and the normal flow of the code can continue. That being said, again: in some cases letting exceptions occur is not bad practice. \$\endgroup\$ – Abbas Oct 17 '14 at 7:39
  • 1
    \$\begingroup\$ Ah yes, I had a feeling I should add a note about that case (which I just did). I use Java, and we don't have an equivalent of TryParse. Hence we test parse and catch the exception to know if the input is parsable. \$\endgroup\$ – ADTC Oct 17 '14 at 9:13
7
\$\begingroup\$

In my opinion there are two sides of your story here:

1. Wrapping exceptions

This can be useful in situations where different type of exceptions may occur and you want to distinguish them, certainly when you want to give specific error-messages for each type of exception. As you have read yourself, when doing so it's best to follow the guidelines Microsoft has given.

This was just a small opinion-based point but for me when reading your question, following point is more important to me:

2. To Exception or not to Exception

First of all, here's the definition of the exception in the .NET Framework (from MSDN):

An exception is any error condition or unexpected behavior that is encountered by an executing program. Exceptions can be raised because of a fault in your code or in code that you call (such as a shared library), unavailable operating system resources, unexpected conditions the common language runtime encounters (such as code that cannot be verified), and so on.

If you read this carefully, you'll understand that an exception occurs when something unexpected happens, something you do not have control of (most of the time).

In your example, the fact that for a given ID, no user exists is not an unxpected behavior of code. It just means that there is no data in the database for that ID. When trying to get an object with given ID (which will obviously return null) and perform some method on it or get a value from a property, this will result in an exception. In this case this will be a NullReferenceException.

I suggest you rewrite your code to following:

public Record GetRecord(string id)
{
    var foundRecord = _dbContext.Records.FirstOrDefault(r => r.Id == id);
}

When no record for given id is found, the default of the type will be returned. Since Record is a reference type, this will be null. Now all you have to do is a null -check when using this method. What you also can to is work with the out parameter and make the method return a boolean. This makes it clean to do the check:

public bool TryGetRecord(string id, out Record record)
{
    record = records.FirstOrDefault(r => r.Id == id);
    return record != null;
}

And then the usage:

Record  r;

if(TryGetRecord(1, out r))
{
    //Record found, use it!
}
else
{
    //Notify the user that no record was found for given ID
}

I know I have strayed from the Exception subject but I thought this might help you too refactoring your code. Keep in mind that exceptions are for when your code does something unexpected. Ecxeption-handling is for catching situations which might result in abnormal behavior of your program, not for data that you (not the code) expect there to be. I hope you understand what I mean and that this helps! ;)

\$\endgroup\$
  • \$\begingroup\$ I like how you use the out parameter to test getting the record. I'm sad Java doesn't support it, though it can be simulated using some final collection object or some sort of fishing net dipped into the method (but that being too much work and hard to maintain, we prefer to just null-check instead). \$\endgroup\$ – ADTC Oct 17 '14 at 3:01
  • \$\begingroup\$ BTW, it's not always a bad thing to let an exception be generated in the normal course of application run. I have expounded on an use case in my answer where it can actually be better to let an exception be generated than to pre-validate data to avoid it. \$\endgroup\$ – ADTC Oct 17 '14 at 3:22
7
\$\begingroup\$

There are, essentially, two things that could happen to a thrown exception:

  • It could be handled further up the call stack
  • It could remain unhandled, either bubbling up until it causes the application to crash, or being swallowed and logged (with maybe some very general, high-level steps taken to deal with any possible invalid state it's left behind)

So, let's think about both of these, and how exception wrapping fits into them.

Unhandled

For an unhandled exception, there are- in general- three important pieces of information it provides us:

  • A stack trace
  • A message
  • An exception type

In fact, at this point the exception type is really just a particular piece of human-readable information, and the stack trace is just further information about where the problem occurred. So really, all three are the same thing- diagnostic information for the person who has to come along and understand what went wrong.

So here, the purpose of wrapping an exception is to provide better diagnostic information. Maybe there's some variable which is likely to be relevant to trying to work out what happened. Specifying a specific type of exception could give important information too (knowing, for example, that the problem arose from the database).

With this in mind, let's compare the two exceptions in your example:

  1. ApplicationException(string.Format("No record found with id: {0}". id), ex);
  2. ApplicationException("An error occurred while processing the record"), ex);

Both are ApplicationExceptions, and in neither case is that really very helpful. But the first provides useful information: that a record was not found, and the id of this record. That immediately means that a person debugging could look at the state of the database, or try to understand why an incorrect id was passed to this method. While somebody could work out that no record was found from the stack trace alone, there's no possible way they'd be able to get to the id, so really the message is just a human-readable way of presenting that potentially vital piece of information.

The second, on the other hand, provides nothing useful. "An error occurred", well, we already knew that. "While processing the record"- that's just a much more vague restatement of what we'd get from the stack trace.

Handled

Here's where it gets more interesting. When we throw an exception that might be handled, there's a new, very important, concern: abstraction level. And this is what the MSDN quote is getting at.

As an example, let's take the good old "repository" abstraction, where we have an IRepository which hides the consumer from having to worry about how data is persisted. Yes, in reality it's more often than not unrealistic that we'd write an application where we needed to swap in and out totally different types of persistence, but it makes for a nice example.

So take two concrete repository classes: SqlRepository and FileRepository. The former stores a Record as a row in a table in the database, keyed by the id. The second stores the Record as serialized text in a text file in a particular directory, named {id}.txt.

Now let's look at our GetRecord method. What happens if I pass an invalid id to either of these? Well, they both throw exceptions, but they both throw completely different types of exception. One throws perhaps an InvalidOperationException (if using IQueryable like in the example) or SqlException, the other throws a FileNotFoundException.

That makes perfect sense, but the problem is they now get thrown up the call stack, through that IRepository abstraction layer, and suddenly our abstraction is leaky. No caller could hope to handle these sensibly without also knowing what all the possible implementations of IRepository are. In fact, they not only need to know the broad classes, but very specific implementation details. Does a SQL-based repository throw InvalidOperationException or SqlException? Does a file-based repository throw FileNotFoundException or DirectoryNotFoundException?

So in order to maintain their abstraction, the repositories should all throw exceptions that can be understood by the calling code. If they need to signal a particular problem with a particular type of exception, this should be a common exception, which describes the problem at an abstraction level understood by the catcher ("Record not found") rather than a level understood by the thrower ("File not found", "Sql record not found").

Going back to the examples in your question, neither of them do this well. Useful information is only provided in a message. Assuming we're not going to try to parse that message to get useful information out of it (which would be a dreadfully complicated and error-prone way of going about things), no caller can do anything useful with these exceptions. ApplicationException does not specify anything about what went wrong.

On the other hand, YAGNI is very relevant here. If you control the code which calls this method, and you know that this kind of exception isn't going to be handled, then going out of your way to define custom exception types to then ignore would be pointless. But the key point is that when writing code which throws exceptions that may be handled by its consumers, ensuring that it throws exceptions at the correct abstraction level is a very good reason for wrapping.

Conclusion

That was longer than I anticipated, so just to re-iterate the good reasons:

  • Providing important information for later debugging/diagnostics which is not available from the wrapped exception
  • Ensuring that for exceptions which can potentially sensibly be handled by calling code, the exception provides information at the correct abstraction level

Of these, the latter is far more situational, and in the example in your question, is likely not to be a concern. The former, though, is the key distinguishing feature between the first exception and your "why not also" example.

The answer, then, to "where should we stop" is when you're no longer providing significant value through one or both of the above reasons.

\$\endgroup\$
4
\$\begingroup\$

I think that for your first case, you should follow @Jeroen's advice and use FirstOrDefault since exception handling is expensive, you don't want to throw an exception every time this might happen.

If you really want to wrap exceptions, you shouldn't wrap them with ApplicationException, since it is very abstract. Every exception is an ApplicationException.

Now, if you're still really sure you want to throw exceptions, you should use custom exceptions. Something along the line of RecordNotFoundException when your ID doesn't exist in your data store. This exception makes it crystal clear that the record wasn't found, and the developers using your API will understand it right away (if they don't, you have another kind of problem!).

About the SqlException, I really don't think you should catch it at this level. You should let the client (UI, web service, etc.) decide what to do with the SqlException since you have no control over it.

\$\endgroup\$
  • \$\begingroup\$ Custom Exception FTW! \$\endgroup\$ – Malachi Oct 17 '14 at 13:07
2
\$\begingroup\$

@TopinFrassi kind of hinted towards this, but a record not being found is something that you should be prepared for, so you should code that into the application and not make it an exception.

public Record GetRecord(string id)
{
    try
    {
        _dbContext.Records.First(r => r.Id == id);
    }
    catch(InvalidOperationException ex)
    {
        throw new ApplicationException(string.Format("No record found with id: {0}". id), ex);
    }
}

Instead of doing this you should just throw in an if statement

public Record GetRecord(string id)
{
    if (_dbContext.Records.First(r => r.Id == id).Any())
    {
        return _dbContext.Records.First(r => r.Id == id);
    }
}

But this will call the query twice

You should already have a boolean method that checks the database for that record to begin with, before you even try to call a GetRecord method.

so it should look something like this

public Record GetRecord(string id)
{
    return _dbContext.Records.First(r => r.Id == id);
}

public boolean DoesRecordExist(string id)
{
    retrun _dbContext.Records.First(r => r.Id == id) ? true : false ;
}

var id = "42"; // should be passed in somehow
Record recordIWant;
if (DoesRecordExist(id))
{
    recordIWant = GetRecord(id);
}
else
{
    /*Handle issue*/
}

in the case of trying to retrieve a record that doesn't exist, you should really handle this at the time of input and not at the time of actual record retrieval

so what you should do is alert the user when they enter a Bad ID or make it so the application won't allow the user to pick the item linked to the broken record by not showing it in the first place

\$\endgroup\$
  • 1
    \$\begingroup\$ Probably a bit old, but I'm not sure if I would want to call the DB twice (first checking if record exists and then getting the record). Seems a lot cleaner using FirstOrDefault(), which would return a null if the record doesn't exists. In code you can then do a if(record != null) {} \$\endgroup\$ – Nick De Beer Feb 18 '17 at 21:02
0
\$\begingroup\$

It is a bad practice unless you have a very good reason to wrap them. Don't wrap exceptions unless you are sure both the intention and implementation are justified in the present. Don't wrap exceptions for some future contingency or other unproven gains.

Some examples of when you might want to wrap exceptions:

  • When an exception isn't a real exception but a result, however the library you're using mistakenly used an exception to represent a result. Then you replace the specific exception with a result such as null or false. You should do this as soon as you call, the library and not later.
  • When something uses two implementations outside of your control throw the same actionable exception but each use their own representation. You may wish to wrap each of those two exceptions into a single unified exception. This is a very rare case but sometimes happens.
  • Potentially during an event any exception is actionable or a result. Normally though you want this in the caller, not the callee.
  • When passing exceptions beyond your application. Your application's exceptions may only contain information for the developer and logging. If so when you pass it outside of the application then you don't want to pass along this data. It is common to replace all exceptions except those marked as passthru with exceptions that remove the data for logging once logged and that give a generic message. You typically want to let the user know something went wrong, to contact support and information to help find the logs (usually timestamp minimum, some people might encrypt the log).
  • When the exception can be translated into a specific course of action in layers above from your current layer, but the layer it originated from didn't know that. Quite rare this happens. You can still usually find a way to mark the exception at the source if the source is code you can edit.
  • There are rare cases where you might want to add more meaningful information. For example SQL error occurred while doing X. The problem is with this is people add information before it is deemed useful and there are other ways to do this than wrapping (your exceptions are classes, you can add all kinds of things like a stack of information). Quite often people prioritize putting make up on exceptions rather than the business of identifying and fixing bugs.
  • When receiving an exception from a source outside of your control that you want to replace with a more flexible exception type. This should only occur on the boundary and can introduce unnecessary complexity. Similar to above. Don't wrap an exception of you can add to it. Unfortunately setting the base exception isn't a thing in most languages.

It tends to be an anti-pattern to wrap exceptions. People like to do it because of the notion things should be unaware of underlying implementation. In reality that's either:

  • A problem in discipline. Either not wrapping the exception on demand or not knowing when you're making something knowledgeable of more than one layer down where it shouldn't be. In often cases people might be worrying about exceptions that they shouldn't have thrown to begin with. Quite often you let the user click this control but you don't check pre-requisites before showing it and instead rely on exceptions to present those beautifully to the user.
  • Counter-productive. You end up making a layer that is meant to be unaware of implementation aware of it by wrapping all the exceptions from the layer below to its own.
  • Unnecessary. When you see exceptions that are not handled (they reach the top) except along the way they're wrapped then you didn't do anything at all. You just added loads of new classes, try catches, etc but the same out come arrived. All you did was to rename something conceptually.
  • Ends up destroying/hiding information, making it ambiguous or hard to get at.
  • Often ends up being a case of re-implementing the stack trace with exception wrapping. Ends up being what should be top level exception handling being implemented in lower levels multiple times unnecessarily.

If in doubt don't wrap exceptions until something happens where you have to wrap exceptions as the best possible solution. Otherwise it's premature.

No, it's not a good practice. It's a bad practice. It is also something you might have to do once in a blue moon.

\$\endgroup\$

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