Exceptions in C#, Done Right

*int.Parse

Thanks for taking the time to respond to my article with so much thought and detail. I don’t think we actually disagree nearly as much as you seem to think. But I’d be happy to respond to some of your comments.

I don’t think you’ve adequately made the case against exceptions here.

I am not trying to make a case against exceptions. In fact, I go on to show code that throws even more exceptions! But you want to throw exceptions because assumptions are not being met, not as part of the expected logic flow.

If the controller is trying to retrieve a user and fails, control will be outside of the controller. In most cases, a simple middleware can triage exceptions program wide. For example, in an web call, you can return 400 errors when exceptions of type UserException are thrown.

There are cases where using middleware to triage exceptions is a good idea, and I actually explore one flavor of this in this blog post. What you need to watch out for is when the middleware becomes too tightly coupled to specific implementation details in your code, since those details could change over time and developers may not know they need to update the middleware accordingly.

You also need to be careful about assuming that a particular type of exception always equates to a particular response. For example, if a request was made to /api/user-profile/j2jensen and there’s no user by that id, then a 404 is a better response header. The client can deduce a 404 means the requested user doesn’t exist. Assuming it’s normal for user profiles to be deleted, this would be an ordinary occurrence which you’d expect to encounter in the course of running your application.

Now let’s say you have another controller than handles replying to an article (api/articles/234/reply), and it calls into another class that sends out notifications to users who were @-mentioned in the post, and that class calls into your UserRepository to get the information for the user that was mentioned. In this case, the correct behavior is to skip sending the notification, but continue to save the comment to the article. You shouldn’t throw an exception, and you shouldn’t return a 404. So you want to write UserRepository in a way that forces the calling code to decide what an appropriate response is to the user not being there. Throwing a UserException and using middleware to respond to that UserException would be a bad idea.

Side note: 400 is almost never an appropriate response for anything but your web framework to return, as it implies something was wrong with the request syntax itself. A good general-purpose “something unexpected happened so I couldn’t do that” code is 500.

Do you really want to continue if the user does not exist?

It depends on what you’re trying to do. /api/user-profile/j2jensen clearly cannot complete if that user doesn’t exist. But api/articles/234/reply should totally be allowed to complete–it should just skip trying to send a notification to the nonexisting @-mentioned user.

What about if the user tries to set an invalid email, password, misses a required field. Not only will you have to check the output of GetUser or SetUser, but you’ll have to chase the output of each function all the way down the stack until you can provide some not useful error…I would have to check the output of each of those functions.

I’m assuming with this example that you’re expecting the caller of SaveUser (or something further up the call stack) to know about all of the kinds of exceptions that could be thrown by the methods that SaveUser calls, and catch and handle those exceptions. If you want your code to work correctly, then you need to handle the different possible failures in some way. You still have to do the work of checking the output (exceptions) of those functions: you’re just doing it without changing the SaveUser contract.

I see what you’re saying about being able to keep your controller code cleaner, but I think it’s coming at the expense of maintainability. Ideally you should be able to find an abstraction that allows you to capture what’s generally important about the various types of errors that ValidateUser and AuthenticateUser are aware of, without tightly coupling your top-level code to those implementation details. But either way, I’d say your program is probably more fragile if top-level code is tightly coupled to implementation details in the low-level code that “skip” the middle layer and aren’t represented in the method signatures along the way.

Exceptions can significantly reduce coding effort.

When used correctly, they can significantly reduce debugging effort as well.

I wouldn’t get the rich information from the exception

You can get rich information from a return type. The question is what kind of detail you really want your high-level code to be coupled to. Information Hiding is a useful practice.

Every time I updated one of those functions, I would have to update SaveUser.

If you make a change to the contract that SaveUser establishes with its calling code, then it’s a good thing to have to change its method signature. If you change the types of exceptions that could be thrown by ValidateUser, then presumably you’d want to also change whatever code is supposed to handle those exceptions? What indication would you have that you’d need to change that handling code? None: just your personal knowledge of the code base. On the other hand, if that same change caused SaveUser to change its method signature then any code calling SaveUser would have to be updated in order to compile. This helps you to prevent bugs.

Interestingly, Java is set up so that methods have to either handle any exceptions that can be thrown inside of them or declare that they’ll throw them. So unless you use a specific subclass of exceptions that’s immune to that rule, throwing a new type of exception in ValidateUser would force you to change the method signature for SaveUser and update its calling code before your application would compile. But C# doesn’t provide any compile-time checking for exception handling like this.

Exceptions are easier and more concise to test

Not so. In my experience, testing exceptions is slightly harder than testing returned values. Using custom attributes to test them is also less flexible than using assertions in your code, too. For example, you can’t specify the scope of code where you expect the exception to come from, or validate the values of properties on the Exception besides the Message. I like using TestCases and FluentAssertions to create flexible, readable tests like this:

[TestCase("a", "Password must be at least 8 characters.")]
[TestCase("I am so strong.", "Password must contain a number.")]
void TestInvalidPassword(string invalidPassword, string expectedError)
{
    var user = CreateValidUser();
    user.Password = invalidPassword;
    var validationResult = new UserService().Validate(user);
    validationResult.IsValid.Should().BeFalse();
    validationResult.Errors.Should().Contain(expectedError);
}

Exceptions are just that: An exception to the normal flow of the program. Something unexpected happened. It shouldn’t have happened. You cite that it should not be a regular every day occurrence. I don’t really think that’s true

If something is happening every day that shouldn’t be happening, that’s a sign of buggy code. I don’t want to be on a project where this is considered normal.

how often is an invalid email going to occur if you have client-side validation?

1. “when someone’s doing something bad.”–I’d consider this an exception to the expected flow for most applications, and it may be useful to know if there’s a bad actor trying to take down your app. Throw exceptions.
2. when your client-side has a bug. Maybe validation isn’t firing, or the client sends the request even when validation failed. Maybe your client-side validation forgot to check something it should have. In this case, you’d want to have an exception with useful information to help you recognize that something is wrong with your code.
3. when your client can’t independently validate something (like whether an email is not already in use). This should be a validation your server takes care of, and returns an appropriate result as part of the API contract rather than just failing. Or you could create a separate API endpoint to handle this validation and have the client leverage it as part of its validation. In that case, the save endpoint could throw an exception.

Many of the native .NET functions throw errors for just this type of thing.

Many of the native .NET functions (int.Parse) also have counterparts (int.TryParse) which don’t throw exceptions. You’re expected to choose which one to use based on whether your code should be able to assume the operation will be successful. What you’re not supposed to do is call int.Parse and then catch InvalidOperationException to handle cases where it couldn’t be parsed.

Exceptions can be pretty fast… Don’t pre-optimize. Developer effort is almost always more expensive.

Totally agree. Performance is not among the reasons I have for avoiding Exceptions. Purely thinking in terms of developer time, I recognize that defects tend to cost a lot of developer time, and that cost increases exponentially the longer the defects go undetected. Exceptions’ greatest strength lies in their ability to make defects noticeable and provide context from the point of failure to help you discover the cause of the defect.

Mauro, thanks for your feedback. Perhaps I was unclear about a number of points. Please allow me to explain.

Although your approach could be useful in some scenarios, your user validation implementation suffers security risks that weren’t there in the first place… By directly reporting if a user as been found or not, you expose your system.

I was not assuming that this method’s result is being serialized and sent directly back to a client. The fact that the original code was throwing two types of exceptions implied that the calling code was expected to try/catch those different exception types and respond to them in different ways. Perhaps the code calling this method would have logged some debug messages, or added a counter to the user’s failed login attempts before returning a generic error response to the client. By using a specific return type instead of exceptions, the calling code handles these different validation scenarios as part of its normal logic flow, rather than using try/catch blocks.

Introducing a ValidationResult simply move the problem from checking for null user to checking for ValidationStatus.Valid on the calling method.

You seem to be assuming that returning a null User would have been a valid thing for the original code to do. There’s no valid reason for the original code to ever return null, and the calling code most likely never would have checked for null. An invalid result in the original code would have thrown an exception, which the person calling the code might not have thought to check for. In my suggested code, we’re returning an object which makes it obvious that a failed login is something we expect to happen on a regular basis.

What i really want from a validation routine is the instance of the user. If, for any reason, a user cannot be provided i quit my procedure. An exception is ok.

When you’re trying to validate a user’s credentials, then there’s a high likelihood of failure due to user error. It’s not really acceptable to allow your program to crash in this case. Getting the User is not a necessary step in some larger set of tasks you’re trying to accomplish here: the whole purpose of this routine is to find out if the user’s credentials are correct, and respond accordingly.

In my opinion, the behavior you describe here would be appropriate for a method called GetUser(), which should be expected to return the User. A method called Validate should return the validation result. There’s certainly room for different people to take different approaches, but I will argue that your method names should indicate what you’re expecting to get back as a result.

You don’t know why GetUser returned null, but rethrow an InvalidOperationException. What if the database was simply offline? … Here you throw an exception so you break your own principles… On a correct implementaion i’d probably let GetUser throw or Try catch the method and rethrow if ever it was necessary.

As you suggest, I am allowing GetUser to throw an exception if the database was offline, or if getting the user fails for any other reason. I never expect GetUser to return null. Returning null is, literally, an invalid operation for that method. The purpose of this code is not to handle a situation that we expect, but rather to fail immediately if the system is behaving in a way that our code was not expecting. Throwing an exception under exceptional (i.e. unexpected) circumstances is exactly the principle I’m trying to teach.

On a related side-note, lately I’ve been using Fody NullGuard so I don’t have to write code like this to guard against unexpected null values. This plugin automatically inserts null checks into the compiled assembly for all of my input parameters and return values, so my actual code can happily assume that a method like GetUser cannot possibly return null.

Hi James,

In my opinion, the behavior you describe here would be appropriate for a method called GetUser(), which should be expected to return the User. A method called Validate should return the validation result. There’s certainly room for different people to take different approaches, but I will argue that your method names should indicate what you’re expecting to get back as a result.

You are right, the appropriate name for the method, to me, should be AuthenticateUser.

You called the method Validate, but in facts you are eventually returning a user instance inside ValidationResult. So you are not really just validating.

Since validating a user per-se has no meaning, it’s clear you are trying to authenticate, isn’t it? And exactly because you are authenticating you should not respond with a state, but with a authenticated user instance, and only if everything was ok. This was the chain of thoughs i had.

You could just remove User from the ValidationResult and the example turns out clearer to me.

Obviously, trying to figure out why you structured the example that way, I had to make a few suppositions that may not reflect the scenario you had in mind while writing the example. I just wanted to point out that the example is open to be interpreted differently from what you would have wantend to represent.
I only suggest you to review the first part of the article with a more meaningful, precise and pertinent example.

I hope I have been constructive :)