Throwing exceptions in switch statements when a particular case cannot be handled

Question

Say we have a function that changes the password for a user on a system in an MVC application.

public JsonResult ChangePassword (string username, string currentPassword, string newPassword) { switch (this.membershipService.ValidateLogin(username, currentPassword)) { case UserValidationResult.BasUsername: case UserValidationResult.BadPassword: // abort: return JsonResult with localized error message // for invalid username/pass combo. case UserValidationResult.TrialExpired // abort: return JsonResult with localized error message // that user cannot login because their trial period has expired case UserValidationResult.Success: break; } // NOW change password now that user is validated } 

membershipService.ValidateLogin() returns a UserValidationResult enumeration, which is defined as:

 enum UserValidationResult { BadUsername, BadPassword, TrialExpired, Success } 

As a defensive programmer, I would ChangePassword() method described above to throw an exception if there is an invalid UserValidationResult value from ValidateLogin() :

 public JsonResult ChangePassword (string username, string currentPassword, string newPassword) { switch (this.membershipService.ValidateLogin(username, currentPassword)) { case UserValidationResult.BasUsername: case UserValidationResult.BadPassword: // abort: return JsonResult with localized error message // for invalid username/pass combo. case UserValidationResult.TrialExpired // abort: return JsonResult with localized error message // that user cannot login because their trial period has expired case UserValidationResult.Success: break; default: throw new NotImplementedException ("Unrecognized UserValidationResult value."); // or NotSupportedException() break; } // Change password now that user is validated } 

I have always considered a pattern similar to the last fragment to be superior to best practice. For example, what if one developer receives a requirement that now that users are trying to log in, if for this reason they should first contact the business? Thus, the definition of UserValidationResult updated as follows:

 enum UserValidationResult { BadUsername, BadPassword, TrialExpired, ContactUs, Success } 

The developer changes the body of the ValidateLogin() method to return a new enum value ( UserValidationResult.ContactUs ), if applicable, but forgets to update ChangePassword() . Without exception, in the switch, the user is still allowed to change his password when their attempt to enter the system should not be checked first!

Is it just me, or is it default: throw new Exception() good idea? I saw this a few years ago, and always (after he did it) suggested that this is best practice.

Answer 1

I always make an exception in this case. Think using an InvalidEnumArgumentException , which gives more details in this situation.

Answer 2

It is good with what you have, although the break statement after it will never be deleted, because the execution of this thread will stop when the exception is sent and not processed.

Answer 3

I used this practice before for specific cases where an enumeration lives “far” from it, but in cases where the enumeration is really close and dedicated to a certain function, this seems a little bit.

Apparently, I suspect that refusing debugging is probably more appropriate.

http://msdn.microsoft.com/en-us/library/6z60kt1f.aspx

Check out the second code example ...

Answer 4

I always like to do what you have, although I usually throw an ArgumentException if this is the argument that was passed, but I kind of like NotImplementedException better, since the error, most likely, and not the caller, should change the argument to a supported one.

Answer 5

I never add a break after the throw statement contained in the switch statement. Last but not least, this is a dangerous warning about unreachable code. So yes, that’s a good idea.

Answer 6

I think that such approaches can be very useful (for example, see Mistakenly empty code codes ). It is even better if you can make this default throw be compiled in the released code, for example assert. Thus, you have additional protection during development and testing, but do not charge extra overhead during release.

Answer 7

You declare that you are in a very defensive situation, and I can say that this is overboard. Wouldn't another developer test their code? Of course, when they run the simplest tests, they will understand that the user can still log in, so they will understand what they need to fix. What you do is not terrible or wrong, but if you spend a lot of time on it, it may be too much.

Answer 8

For a web application, I would prefer to by default generate a result with an error message that asks the user to contact the administrator and log the error, rather than throw an exception that could lead to something less meaningful to the user. Since I can expect, in this case, that the return value may be something other than what I expect, I would not consider this truly exceptional behavior.

Also, your use case, which leads to an extra enumeration value, should not introduce an error into your specific method. My assumption would be that the use case only applies at login - what happens before the ChangePassword method can be legally called, presumably since you want to make sure that the person was registered before changing the password - you should never see the value ContactUs as a return from password verification. If used, it will be indicated that if ContactUs results in a return, this authentication fails until the condition that leads to the result is cleared. If it were otherwise, I would expect you to have requirements on how other parts of the system should react in this state, and you should write tests that would force you to change the code in this method to fit these tests and specify new return value.

Answer 9

Checking run-time constraints is usually good, so yes, best practice helps with the crash principle, and you stop (or log in) when an error is detected, not in silence. There are times when this is not true, but given the switch statements, I suspect that we do not see them very often.

To develop, switch statements can always be replaced with if / else blocks. Looking at this from this point of view, we see that throwing vs does not throw out the default switch case, equivalent to this example:

  if( i == 0 ) { } else { // i > 0 } 

against

  if( i == 0 ) { } else if ( i > 0 ) { } else { // i < 0 is now an enforced error throw new Illegal(...) } 

The second example is usually considered better, because you get a failure when the violation of the error is violated, and does not continue with a potentially incorrect assumption.

If instead we want:

  if( i == 0 ) { } else { // i != 0 // we can handle both positve or negative, just not zero } 

Then, in practice, I suspect that the latter case will most likely look like an if / else statement. Because of this, the switch statement resembles the second if / else block so often that throws are usually best practice.

Several considerations deserve attention: - consider the polymorphic approach or the enum method to replace the switch statement in general, for example: Methods inside enum in C # - if throwing is the best, as indicated in other answers, prefer to use a special type of exception to avoid boiler plate code, for example: InvalidEnumArgumentException