Scott Hanselman

Great. So now I have to rename all my "Save" methods to "SaveOrFailSilently".

I definitely agree. If you're stuck then throw.

This is reminiscent of the rule "Don't catch an exception unless you intend to do something about it. Don't swallow an exception unless you've fixed the problem".

Too many people wrap potentially troublesome calls in a

try { ... } catch{}

exception black hole. It's a maintenance nightmare.

The only reason to catch a specific exception at higher level would be to recover from it, otherwise it should just continue to bubble up.

If the recovery code is specific to teh abstraction, it should be encapsulated by the abstraction:

try
{
storageConnection.Save()
}
catch
{
storageConnection.Recover()
throw
}

What is everyone's view on the type of exception to be thrown? Here it makes sense to include the .Net exception inside your own generic StorageException, but what about generally?

Doesn't it make more sense to have your own exception related to the task in hand (and the domain itself) than a generic .Net exception? For example, WebServiceException for anything WS related instead of WebException?

Or is that too much abstraction and pointless?

Good point,

And when throwing exceptions is not a option, because of performance consideration, then it is worth to reflect that in the method name. For the Save() example, I would go with TrySave(), just because SaveOrFailSilently() is too verbose. Or the team might settle for a simple naminb convention, e.g. adding OFS postfix as is in SaveOFS :)

I would just like to remind people to be sure to document what possible exceptions can be thrown, even if it's just a list. (the xml docs are good because it shows up in intellisense)

Having wrapped more than my fair share of underlying exceptions as .InnerException properties on my shiny custom exceptions, I can safely say - don't. I know you normally shouldn't treat exceptions like objects, but I think this is one of the cases where you should. What exactly is wrapping a DBException (or FileNotFoundException) in a FooClassException buying you?
The subsuming class might know how to handle a FNFException ("pick a real file this time, dummy") but not a DBException ("uh, something blowed up in SQL Server or something?") and if it's getting a generic StorageException, it's got to peer inside the exception to figure out if it can bubble up or not.
A generic exception certainly could work but it wouldn't be my first choice.

Why not have your function simply return a value that is a confirmation if it succeeded? So Save() would return a boolean value instead of having the function throw an exception. Exceptions are useless information to the end user using the application so the exception message obviously won't be displayed to them. They just need to know if their request to save has succeeded or not.

Renaming the function to TrySave() as Ivan suggested also seems like a better idea than throwing an exception.

It seems to me there are two sorts of exceptions - the 'You violated the contract' exception (ArgumentException), and the 'Something bad and out of my control just happened' (SqlException due to server down). Contract related exceptions I throw early, throw often. For these, there should also be a non-exception way for developers to check validity:

if (storageConnection.IsSaveable) storageConnection.Save(); else ExplainToUserWhyUnsavable(storageConnection);

The exception from Save is then worded such that a developer knows he's violated the contract, and how to fix:

throw new ArgumentException("This Storage Connection is not savable, because a filename has not been specified. Developers should prevent this exception by checking the 'IsSaveable' property before calling this method.")

Depending on the project, this could translate into a full validation framework, like the Validation App Block, or similar.

True unexpected exceptions I have a harder time getting my head around. In most cases, we just let them bubble up to the UI's error handling (making sure to close connections, etc. of course), but I suppose if the caller needed to handle them differently, we would wrap in a custom exception. I think what confuses me here is that I can't imagine many scenarios where the developer could recover from such an exception but not prevent it. If the caller will recover from the exception, then it often means it was preventable (ie most FileNotFoundExceptions can be prevented by checking for the file first).

My business code in applications rarely throw exceptions (only in exceptional cases :). A file not found, is NOT an exceptional case, I would expect it to happen quite often. As I see it the reason you want an exception is to show a pretty message to the user that they picked a file that does not exist, is read-only, or whatever file system issue occured. That is NOT exceptional. You can sometimes prevent these exceptions from happening in the first place with some defensive coding such as checking that a file exists before attempting to open it, or checking that it is not readonly before attempting a save, or by validating that a variable is not null before passing the value into a method call.

Essentially you want a way for the implementation code of the storage interface to tell you what happened in simple terms so that the user can make a corrective action. But you also want an easy to use programmatic interface. how about creating a container for a collection of messages a various severities that the implementation code can add to as it works. When the operation returns specify if there were any errors. if there were errors, check the message container for what needs to be displayed to the user.

The reason i like this is that as a consumer of this storage service, you don't know what to display to the user after a failure to save. That is the whole point of abstracting it into an interface. You don't care how it gets done, you just want it done, and if it doesn't work you want to know why it failed so you can tell the user. This is exactly what the bool return and message container 'pattern' does.

Here are a few options of what the storage API may look like.

//use this option for instance classes that are used only by one thread.
if (!storage.Save())
{
DisplayMessagesOnPage(storage.Results);
}

OR
//use this option if the storage class is shared across requests (such as with static methods)
ExecutionResults results = new ExecutionResults();
if (!storage.Save(results))
{
DisplayMessagesOnPage(results);
}

There is still a possibility for exceptions in this scenario. For instance, the API may have been used incorrectly. Maybe a required parameter was not specified, was null, or was invalid. Or maybe the method call was invalid given other state within the storage provider. Or maybe the storage provider wasn't configured properly. There are built in exceptions for all these cases in the .net framework that you can throw (ArgumentException, ArgumentNullException, ArgumentOutOfRangeException, InvalidOperationException, ConfigurationException).

Other cases for letting an exception bubble up would be things like StackOverflowException, and other system level issues. It's hard to anticipate these, so the tempatation is to catch Exception and take the Exception.Message and put it in your message container and return false. But Don't!

I don't recommend creating custom exceptions to return a pretty message with stack data (InnerException). Make up your mind in each case and either let the exception bubble or return false with an explanation that can be given to the UI.

For the example quoted (the Save method with pluggable back ends) I would definitely agree with Scott the wrapping the underlying exception in a StorageException or some such is the way to go. The point being that the consumer doesn't care about the underlying implementation. Letting a FileNotFound or DBException bubble us exposes the underlying implementation, about which the consumer can often do nothing, and shouldn't care about anyway. The reason you wrap the real exception is so that someone doing forensics later can figure out what really went wrong.
The other big advantage to wrapping is that when the next module comes along and you start using FTP to provide the underlying storage, the client doesn't have to add a new handler for NetworkExceptions, since they only care about the StorageException.
I realize this is an area in which everyone has their personal preference, but mine is to always assume success (rather than checking HRESULTs everywhere) and then dealing with the exceptions that come up. Catch the ones that you can do something about, and let the rest go. I'm still traumatized by memories of 25-level-deep if(result == S_OK) blocks from C++ COM programming. Let's put that behind us and get one with writing code that provides business value.

Will the supporters of the Boolean result + message container (I'm astounded they still exist) PLEASE explain how that is possibly better than throwing an exception? Even if there are performance hits (I'd want to see documentation for that - and bear in mind that exceptions are much more expensive when attached to the debugger than in normal runtime, so they might seem slower when you're running from Visual Studio), I don't see how they could possibly be enough to affect a normal user file save.

All it means is you have to maintain two completely different paradigms of error handling - the exception, and the boolean-message check. Not to mention issues with message contain consistency. And the boolean-check message container is FAR more vulnerable to sloppy coding - forgetting to check the boolean, nesting the same object (thus wiping out the message container), etc.

If you know you want to return a user friendly message to pop up in a message box, just raise a UserFriendlyMessageException or whatever. I mean, there's NO WAY to capture every possible method - sure, you might catch FileNotFound.... but what about File Locked? Network Drive Unavailable? Insuffucient permissions? Etc etc etc. How do you determine what gets caught by the Save boolean and what bubbles up?

The Exception language feature was designed exactly for handling these situations. I find it amazing that people still get hung up on the semantics of whether something is "exceptional".

To clear up my earlier post, Boolean result + message is usually not better than exception for "error handling". However, it has usefulness when dealing with rule validation and/or actionable reasons why the requested operation cannot be performed. I think I cited numerous situations where you should let the exception bubble, or throw a new exception yourself. The only exception I don't like is one that is meant to throw up a pretty message to the user.

You can find numerous performance tests of exceptions. If you don't belive them create your own test. Just search google, "performance implication of Exception" (or MSN search for you Microsoft employees). Performance is not the reason I suggest boolean + message. However, it is a compelling reason not to throw an exception just to pass any possible message to the calling code.

If an exception makes it to your UI code, (1) log it, (2) show the user an error page with a general hardcoded message (such as "system error, try again later") with zero details about the actual exception. The last thing you want are your end users knowing any details about your application code (especially in web sites).

If a business rule is broken, you want to display a meaningful message the user can take action on. Most of the time you just can't fit that into an exception Message property. This is when you return false with some means to recover the message that the UI can display.

Boolean + message is not the same as an hresult. All the caller code needs to do is display the message, no matter what caused the method to return false. There is no branching of code to show different messages for different results, as it is simply true (do nothing) or false (show the detailed message).

Is the disagreement is because we aren't seeing eye to eye on what is actionable by the user?

I want to make one more distiction between when to use boolean + message or exception.

You want to be real careful that your component is consistent. If you tell the consumer to put OurCustomException.Message on the page to tell the user what went wrong, how do they know not to do that for all your component's custom exceptions? Keep in mind there can be sensitive information in an exception about the server such as connection string, server name, ip addresses of routing servers, or sensitive data such as passwords, credit card numbers, SSN's, etc... You really don't want a malicious user to be able to get ahold of that information.

The messages returned in the boolean + messages pattern should always targeted to the end user, and should never contain any sensitive information (even about themselves, as that lowers confidence of security in your product).

With this distinction, you are telling the component consumer what is safe to display and what may not be. It defines a very clear pattern to follow.

I wonder how many developers take the Message property from any exceptions and display them on the screen?

I've seen this called the Samurai Principle:
http://c2.com/cgi/wiki?SamuraiPrinciple

The problem many folks here are missing is that when writing a component library it is usually the right design decision to not know anything about a particular UI implementation that will be consuming that component. So while these arguments about using bool/message container _may_ hold up for top-level business logic you don't want to couple normal components to the UI. Say you're building an FTP transfer component, you don't know if your UI is an FTP program with a WinForms UI or a server component that should do some logging. The information and context you'd provide to the FTP program should be decided by the FTP program and not the library itself. So you don't want your FTP transfer component spewing up text messages, you want it returning error conditions (read exceptions with some specificity, perhaps an error code or other inner context like a nested exception) that the developer of the FTP WinForms app can look for to allow it to decide on the proper messaging and UI actions to perform. Otherwise, you end up with your component logic tightly bound to a particular UI which makes your code harder to reuse in other cases and then you end up copy/pasting logic all over the place which is definitely not the desired result.

I put my thoughts to blog here a while ago:

http://www.thejoyofcode.com/exceptions.aspx

I agree with the idea of wrapping the underlying exception and throwing a custom exception in this case. My thoughts align with Patrick Cauldwell's fairly closely. However, I would add that an important part of object-oriented programming and making your code reflect your intention. If your intention is to save something, then your method should save it and not return anything. If something out-of-the-ordinary happens while trying to save, use exceptions. That's why they are provided. That way it is very clear an exception has occurred to other developers looking at your code. Using a custom exception will not only add more clarity to your code, but it also allows the consumer of your method to check for an exception specific to the domain of your app.

Just my $0.02.

I agree with Mark Zuber and Josh 100%. Josh put it really well in his referenced blog article. Here is a quote from his summary.

<In Talking about returning null from a Find method when a match is not found>
"This isn't a 'return code', it's the result. "

You could argue that the .net framework is very inconsistent in use of exceptions. As Josh noted, "Dictionary<> class will throw a KeyNotFoundException if you pass a key that doesn't exist in the dictionary, whereas the Http Cache just returns null if a key doesn't exist". If you see these differences as a problem, then you don't understand the contract in each case.

The decision involves taking into account what defines your contracted result. In many cases that Mark and Josh cite, the result is that the action either works or it does not for various reasons. In those cases, anything but an exception would essentially be a return code. In other business cases, such as the user entering an invalid value in a textbox, throwing an exception would be a terrible idea because they could not recover from their mistake.

I think everyone came to this discussion with specific examples in mind that do have a specific best answer. However, developing an application is not a cookie cutter process. The same answer does not apply to every situation, and each methodology has its merits in different situations.