views:

76

answers:

5

As you can see in the MSDN StringValidator documentation, the Validate method returns void.
If validation doesn't succeed the Validate method throws ArgumentException.
I thought that "You only throw an exception when something exceptional happens".
Surely a validator that failed to validate isn't exceptional..
Why not return bool? What am I missing here?
Is this a "style issue" (i.e. if it was returning bool, it would still be correct, but just different style)?

Note: Method CanValidate may have something to do with it, but I still see no reason for this behavior.

+1  A: 

I agree that it should return a boolean, failure to validate is not exceptional in many real world application.

Paul Creasey
Actually, the explanations for the use of exceptions are quite compelling!
Paul Creasey
A: 

There could be cases where an application is architected in such a way that it would be an 'exceptional event' for the validation to fail at the point where this code is being called; where the calling code simply wants to do one, last double-check quickly and just "assume it's all OK" - and yet still be able to be notified if something unexpected has happened to cause the value to fail validation.

I would say those cases might be uncommon, but...

Andrew Barber
+2  A: 

see ArgumentException Class

Can you use bool to replace the info that ArgumentExeption may contain? And based on this page,

ArgumentException: "The exception that is thrown when one of the arguments provided to a method is not valid.

So if the argument "Object" in StringValidator.Validate(Object) is not valid, what should be the best choice? Return a whole variety of objects or just throw ArgumentException?

Bolu
IMO wanting to return more info doesn't justify throwing an exception. You can return a whole variety of objects to inform the user what happened (including custom ones).
Oren A
@Oren: ArgumentException: "The exception that is thrown when one of the arguments provided to a method is not valid." you tell me if the argument "Object" in StringValidator.Validate(Object) is not valid, what should be the best choice?
Bolu
@Bolu: Do **you** throw an ArgumentException whenever you get invalid arguments in your methods?
Oren A
@Oren: No, but StringValidator.Validate method is all about validating its argument, so at here just throw ArgumentException is a good practice.
Bolu
Bolu may or may not, but I most certainly do. Not all the time, sometimes it's pointless (attempting to continue without checking will have an ArgumentException thrown by code I call anyway) sometimes it's not truly exceptional (close the the UI in particular), sometimes it shouldn't have happened (internal and private methods where the calling code should have already checked), but sometimes it is indeed appropriate.
Jon Hanna
A: 

StringValidator, along with any type derived from ConfigurationValidatorBase are intended to be used with the .NET configuration system (see Jon Rista's article for a great overview of its capabilities). A simple use case is as follows:

public class MyConfigurationSection : ConfigurationSection
{
    [ConfigurationProperty("ConfigurationText", IsRequired=true)]
    [StringValidator(0, 10)]
    public string Text { get; set; }  // implementation removed for brevity
}

In this case, the StringValidatorAttribute instantiates a StringValidator internally, which derives from ConfigurationValidatorBase. Consequently, any type of ConfigurationValidatorBase may be used with a configuration property (given that the types of the property and what validated match), and so I believe the configuration system needs to interact with this abstraction.

If the validation method of ConfigurationValidatorBase always returned a Boolean value, how is it possible to determine what the specific validation error is, for any property, for any type of validation? I'm sure the type could have been designed to return both a string and Boolean, but then it would be difficult to get additional information from the error as the string would need to be parsed (i.e., the value causing the length validation error).

Using an exception seems to be a good mitigating solution without introducing additional complexity to the system.

Steve Guidi
+1  A: 

You are correct when you say:

Note: Method CanValidate may have something to do with it

Between the two, the user of this code has the ability to receive both a non-exceptional response to a check for validation, and also to trigger an exception. That both of these options exists suggests that the designers of the class see at least some reasonable cases for each approach. Assuming their design isn't entirely boneheaded (or even without that assumption, since it applies elsewhere), lets consider cases where both make sense.

Let's consider a method static MySettings ReadSettings(string filePath). This method will obtain some sort of meaningful object from the contents of a file. Let's also imagine that we do every bit of this "close to the metal", without depending on the file-operation classes in the BCL; not because that's a good idea, but because it brings all issues of exception handling right into the realms of what we have to deal with (in reality we can lump several of the exceptions we're going to consider together by allowing the BCL file-related methods to either return us a usable stream or else throw an exception for several possibilities, but we want to consider those possibilities here).

Possible things that can go wrong are:

  1. filePath is not valid.
  2. The file doesn't exist, or cannot be opened for another reason.
  3. The file does not contain appropriate data.
  4. We try to open a file to which the OS restricts access for security reasons.
  5. We try to open a file to which the OS does not restrict access (it doesn't fall under a general security rule), but which it would be a security breech to open (e.g. it belongs to a different "user" under the rules of our application, but not under OS rules).
  6. File operations break part way through (e.g. disk error).
  7. An unexpected error happens, that we haven't considered yet.
  8. An unexpected security error happens, that we can't catch because we haven't considered it (essentially, there is a security hole in our code).

Notably, 7 and 8 are not cases we are explicitly coding for; by their nature they are things we haven't thought to code for.

Now. Some of this is impossible to completely avoid; e.g. even if we check for existence of a file, that introduces a race condition (it can be deleted afterwards) and doesn't preclude disk-error part of the way through reading it.

Let's consider what using code will do in this case. It's going to be pretty rare, so possibly using code will fail to check for it (you may argue that's imperfect, but assuming perfection in calling code is poor design for your code). It may be that there is a clear action that could be taken in this case (e.g. if the calling code is close to the UI then it can respond with a message to the user), but it may be that there isn't, which forces it to in turn respond to the code that called it with a signal that the operation failed, along with possibly doing so for other things that can go wrong at that level (simply returning a boolean of success is of little value in knowing why something failed, and what should be done with it). So, we need a mechanism for signalling the failure that can go all the way up the call stack until we get to something that can either:

  1. Write off the loss (eating exceptions is generally a bad idea, but in rare conditions it makes sense, so we'll include this in our list).
  2. Report to the user, perhaps seeking user-guidance on what to do next.
  3. Retry in some other way.
  4. Shut down some functionality (if e.g. we're trying to load plug-ins).
  5. Log an exception.
  6. Shut down the entire application.
  7. A whole bunch of other application-specific ways that one might want to deal with the situation.
  8. A combination of the above.

When writing 'ReadSettings' we have not just no idea as to which of these will be appropriate, but more importantly no idea of how far the code that does this will be from our code, and no idea of what else could go wrong in the set of operations of which ours are only one part. The description of what we need above (ability to climb up the call-stack, descriptiveness, ability to be mixed with dealing of similar signals from other methods) is a reasonably good description of precisely what the exception mechanism gives us. So when any of the possibilities above happen we throw an exception, apart from the last two possibilities (that we haven't planned for), though ideally we can make it so that this also throws an exception.

First approach at planning this, our set of procedures are:

  1. Obtain a file handle based on the path (exception if failure).
  2. Open the file (exception if failure).
  3. Read the data (exception if failure).
  4. Parse the data (exception if failure).
  5. Build the MySettings object (exception if failure).
  6. Check MySettings object is reasonably valid (exception if failure).
  7. Return the MySettings object.

There are problems here. First, our attempt to obtain the file handle depends on failing if the handle is invalid. This works but may be more expensive that testing first (depending on how much I/O work has gone on before the failure), and much more importantly can introduce the possibility of our blindly doing something stupid at the I/O level which could introduce a security issue (what sort of issue? I don't know, and if I deal with validation first, I don't even have to know). Hence we introduce step 0, which is validating the filePath. We could make this a check followed by an explicit exception-throw right in our ReadSettings, but the same rules for validation probably exist elsewhere (e.g. WriteSettings), so to avoid duplicated effort and (more importantly for correctness) to centralise the validation logic, we might have a void CheckValidSettingsPath(string filePath) method that throws an ArgumentException if the path isn't valid.

We can also now get away from the madness of doing all the file operations at a low level, by using BCL classes and depending on their failing for many of the possible failure conditions. One bonus of this is that this may well take out a few (or even a large swathe) of "cases we haven't thought of", since the BCL file operation classes may have thought of them even if we didn't!

We can also have some invariants in the MySettings class, so we can offload some of the responsibility for checking correctness to there. We now have:

  1. Validate path (exception if failure).
  2. Read the data (exception if failure - BCL does it for us).
  3. Build the MySettings object (exception if failure - MySettings does if for us at least some of the time).
  4. (Optional) checks on MySettings beyond those inherent to all MySettings objects (exception if failure).
  5. Return the MySettings object.

We've reduced the complexity. An issue remains as to whether we should just allow these exceptions to pass through, eat them and throw our own, or wrap them in a new one of our own. As a rule I would allow most to pass through, but in public methods of public classes wrap them in a new one (so on the one hand the user of the code sees ReadSettings failing rather than a private CheckValidSettingsPath she's never heard of, but on the other, the details of what went wrong at the lower level is still available [there may sometimes be security reasons for hiding this]); eating and throwing a "fresh" exception of ones own (no innerException) works well only when it is sure to be able to completely explain what went wrong.

So, this is a case comparable to StringValidator.Validate. Indeed, it could well be used as part of our CheckValidSettingsPath method.

Let's now consider the case where we are writing code that calls ReadSettings. Obviously we have a string that contains a filePath:

Possibly, (because of the nature of how we got that string) the chances of it being invalid are either nil, or could only happen if something has gone seriously wrong elsewhere in the application. In this case there is no point doing any validation here; we want to just try and let the exception happen if it will.

Possibly, there's not much we can do in the case of a failure, except for have that failure happen anyway. Again, we just try and let the exception happen if it will.

Possibly, we want to know we've a reasonable chance of ReadSettings succeed, before we do several steps of which ReadSettings is not the most immediate (i.e. check it is likely to work, then do several other things, and then read the settings). Here an ability to check validity first is useful, and we then throw an exception when it happens. (There are several reasons why, if you are going to fail, it's good to fail early, which I won't go into).

Finally, possibly we are close to the UI level and want to present a user-message ASAP in the case of the user presenting us with an obviously invalid input.

In these last two cases we can benefit from a method that operates much like CheckValidSettingsPath but which returns a boolean rather than throws an exception. In the first few possibilities, having an invalid path is either truly exceptional, or as close to it as makes no practical difference. In the last two possibilities, having an invalid path is not exceptional, and should ideally not be treated as such.

Note that there is still the possibility that after we've checked the path is valid, the ReadSettings still goes wrong, so we will combine both the explicit-check-with-boolean approach and the implicit-check-during-operation approach that throws an exception.

It is for cases like the last two that StringValidator.CanValidate exists, and again it could well be used of part of a IsValidSettingsPath method.

Jon Hanna