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:
filePath
is not valid.
- The file doesn't exist, or cannot be opened for another reason.
- The file does not contain appropriate data.
- We try to open a file to which the OS restricts access for security reasons.
- 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).
- File operations break part way through (e.g. disk error).
- An unexpected error happens, that we haven't considered yet.
- 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:
- 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).
- Report to the user, perhaps seeking user-guidance on what to do next.
- Retry in some other way.
- Shut down some functionality (if e.g. we're trying to load plug-ins).
- Log an exception.
- Shut down the entire application.
- A whole bunch of other application-specific ways that one might want to deal with the situation.
- 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:
- Obtain a file handle based on the path (exception if failure).
- Open the file (exception if failure).
- Read the data (exception if failure).
- Parse the data (exception if failure).
- Build the
MySettings
object (exception if failure).
- Check
MySettings
object is reasonably valid (exception if failure).
- 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:
- Validate path (exception if failure).
- Read the data (exception if failure - BCL does it for us).
- Build the
MySettings
object (exception if failure - MySettings
does if for us at least some of the time).
- (Optional) checks on
MySettings
beyond those inherent to all MySettings
objects (exception if failure).
- 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.