tags:

views:

369

answers:

6

I've taken the advice I've seen in other answered questions about when to throw exceptions but now my APIs have new noise. Instead of calling methods wrapped in try/catch blocks (vexing exceptions) I have out argument parameters with a collection of errors that may have occurred during processing. I understand why wrapping everything in a try/catch is a bad way to control the flow of an app but I rarely see code anywhere that reflects this idea.

That's why this whole thing seems so strange to me. It's a practice that is supposedly the right way to code but I don't see it anywhere. Added to that, I don't quite understand how to relate to client code when "bad" behavior has occured.

Here's a snippet of some code I'm hacking around with that deals with saving pictures that are uploaded by users of a web app. Don't sweat the details (it's ugly), just see the way I've added these output parameters to everything to get error messages.

public void Save(UserAccount account, UserSubmittedFile file, out IList<ErrorMessage> errors)
{
    PictureData pictureData = _loader.GetPictureData(file, out errors);

    if(errors.Any())
    {
        return;
    }

    pictureData.For(account);

    _repo.Save(pictureData);
}

Is this the right idea? I can reasonably expect that a user submitted file is in some way invalid so I shouldn't throw an exception, however I'd like to know what was wrong with the file so I produce error messages. Likewise, any client that now consumes this save method will also want to find out what was wrong with the overall picture saving operation.

I had other ideas about returning some status object that contained a result and additional error messages but that feels weird. I know having out parameters everywhere is going to be hard to maintain/refactor/etc.

I would love some guidance on this!

EDIT: I think the user submitted files snippet may lead people to think of exceptions generated by loading invalid images and other "hard" errors. I think this code snippet is a better illustration of where I think the idea of throwing an exception is being discouraged.

With this I'm just saving a new user account. I do a state validation on the user account and then I hit the persistent store to find out if the username has been taken.

public UserAccount Create(UserAccount account, out IList<ErrorMessage> errors)
{
    errors = _modelValidator.Validate(account);

    if (errors.Any())
    {
        return null;
    }

    if (_userRepo.UsernameExists(account.Username))
    {
        errors.Add(new ErrorMessage("Username has already been registered."));
        return null;
    }

    account = _userRepo.CreateUserAccount(account);

    return account;
}

Should I throw some sort of validation exception? Or should I return error messages?

+7  A: 

Despite the performance concerns, I think it's actually cleaner to allow Exceptions to be thrown out of a method. If there are any exceptions that can be handled within your method, you should handle them appropriately, but otherwise, let them bubble up.

Returning errors in out parameters, or returning status codes feels a bit clunky. Sometimes when faced with this situation, I try to imagine how the .NET framework would handle the errors. I don't believe there are many .NET framework methods that return errors in out parameters, or return status codes.

Andy White
I've been in the same school of thought. Other answered questions around this topic usually have responses like "only throw an exception if there's nuclear holocaust". That's why I've tried this experiment, to see what coding is like without using exceptions to control flow.Check out the new snippet I added to my question. It's a somewhat "softer" set of erroneous behaviors than dealing with file loading, etc. Does that change the perspective at all?And while out parameters seems clunky, I do admit that having to put try/catch on everything is clunky in its own right.
Nick Swarr
The key difference is that the .NET framework does not have any business logic in it. It is a framework, so anything outside the norm is an exception. In domain logic, there are varying degrees of "outside the norm". Is it outside the norm if a username is taken? I would say no.
Rex M
@Rex, that is a good point. I think the difficulty comes with making these decisions when business logic is involved. I guess it just seems more intuitive to me to a assume a method did what it was supposed to, unless an exception was thrown. I'd rather not have to worry about checking for a null return value, or checking/handling error codes on a status object. I think a low-level method like "CreateUser" might not know what to do if the user name was already taken, so it's best to throw an exception and let higher-level code decide how to handle that.
Andy White
However, one case where it might make sense to return a status object would be a web-service that can have a whole host of different types of errors. Rather than forcing the user to handle SOAP faults, it's often cleaner to catch all exceptions on the server-side, then return an appropriate error code to the caller.
Andy White
+2  A: 

I think this is the wrong approach. Yes, it's very likely that you'll get occasional invalid images. But that's still the exceptional scenario. In my opinions, exceptions are the right choice here.

Matthew Flaschen
A: 

I would allow for exceptions as well but based on your thread your looking for an alternative. Why not include a status or error information in your PictureData object. You can then just return the object with the errors in it and the other stuff left empty. Just a suggestion, but you are pretty much doing exactly what exceptions were made to solve :)

Kelsey
+4  A: 

By definition, "exception" means an exceptional circumstance from which a routine cannot recover. In the example you provided, it looks like that means the image was invalid/corrupt/unreadable/etc. That should be thrown and bubbled up to the topmost layer, and there decide what to do with the exception. The exception itself contains the most complete information about what went wrong, which must be available at the upper levels.

When people say you should not use exceptions to control program flow, what they mean is: (for example) if a user tries to create an account but the account already exists, you should not throw an AccountExistsException and then catch it higher up in the application to be able to provide that feedback to the user, because the account already existing is not an exceptional case. You should expect that situation and handle it as part of your normal program flow. If you can't connect to the database, that is an exceptional case.

Part of the problem with your User Registration example is that you are trying to encapsulate too much into a single routine. If your method tries to do more than one thing, then you have to track the state of multiple things (hence things getting ugly, like lists of error messages). In this case, what you could do instead is:

UsernameStatus result = CheckUsernameStatus(username);
if(result == UsernameStatus.Available)
{
    CreateUserAccount(username);
}
else
{
    //update UI with appropriate message
}

enum UsernameStatus
{
    Available=1,
    Taken=2,
    IllegalCharacters=3
}

Obviously this is a simplified example, but I hope the point is clear: your routines should only try to do one thing, and should have a limited/predictable scope of operation. That makes it easier to halt and redirect program flow to deal with various situations.

Rex M
Check out my last snippet. Is that something that would warrant a thrown exception?
Nick Swarr
No, not an exceptional case. You should handle something like a username already in use as part of your business logic using something like a BrokenRules collection.
JasonS
Rex, for the user registration method, it's at the level of the orchestration of creating a new account. Part of that orchestration is validating the health of the account and then passing it on to the appropriate persistent store. Assume that there is no username check. We only validate the state of the account before saving it. How do I relate the state errors back to the client? Is the recommendation to push the validation up to a higher level module (in this case, a controller)?
Nick Swarr
@Nick if you want to keep the entire user process at a deeper level of abstraction, the entry routine should not return a UserAccount object as in your example, but a CreateUserResult object which contains details about what happened (success/failure) and, if applicable, the user account as a sub-property.
Rex M
I flirted with this idea and I think it provides the cleanest solution. I won't throw an exception for non-exceptional behavior, I'll relate issues back to the client, I'll have a "result" class I can easily maintain and I won't have the super hideous out parameters.
Nick Swarr
A: 

First off, exceptions should never be used as a control-flow mechanism. Exceptions are an error propagation and handling mechanism, but should never be used to control program flow. Control flow is the domain of conditional statements and loops. That is quite often a critical misconception that many programmers make, and is usually what leads to such nightmares when they try to deal with exceptions.

In a language like C# which offers structured exception handling, the idea is to allow 'exceptional' cases in your code to be identified, propagated, and eventually handled. Handling is generally left to the highest level of your application (i.e. a windows client with a UI and error dialogs, a web site with error pages, a logging facility in the message loop of a background service, etc.) Unlike Java, which uses checked exception handling, C# does not require you to specifically handle every single exception that may pass through your methods. On the contrary, trying to do so would undoubtedly lead to some severe performance bottlenecks, as catching, handling, and possibly re-throwing exceptions is costly business.

The general idea with exceptions in C# is that if they happen...and I stress if, because they are called exceptions due to the fact that during normal operation, you shouldn't be encountering any exceptional conditions, ...if they happen then you have the tools at your disposal to safely and cleanly recover and present the user (if there is one) with a notification of the applications failure and possible resolution options.

Most of the time, a well written C# application won't have that many try/catch blocks in core business logic, and will have a lot more try/finally, or better yet, using blocks. For most code, the concern in response to an exception is to recover nicely by releasing resources, locks, etc. and allowing the exception to continue on. In your higher level code, usually in the outer message processing loop of an application or in the standard event handler for systems like ASP.NET, you'll eventually perform your structured handling with a try/catch, possibly with multiple catch clauses to deal with specific errors that need unique handling.

If you are properly handling exceptions and building code that uses exceptions in an appropriate way, you shouldn't have to worry about lots of try/catch/finally blocks, return codes, or convoluted method signatures with lots of ref and out parameters. You should see code more like this:

public void ClientAppMessageLoop()
{
    bool running = true;
    while (running)
    {
        object inputData = GetInputFromUser();
        try
        {
            ServiceLevelMethod(inputData);
        }
        catch (Exception ex)
        {
            // Error occurred, notify user and let them recover
        }
    }
}

// ...

public void ServiceLevelMethod(object someinput)
{
    using (SomeComponentThatsDisposable blah = new SomeComponentThatsDisposable())
    {
        blah.PerformSomeActionThatMayFail(someinput);
    } // Dispose() method on SomeComponentThatsDisposable is called here, critical resource freed regardless of exception
}

// ...

public class SomeComponentThatsDisposable: IDosposable
{
    public void PErformSomeActionThatMayFail(object someinput)
    {
        // Get some critical resource here...

        // OOPS: We forgot to check if someinput is null below, NullReferenceException!
        int hash = someinput.GetHashCode();
        Debug.WriteLine(hash);
    }

    public void Dispose()
    {
        GC.SuppressFinalize(this);

        // Clean up critical resource if its not null here!
    }
}

By following the above paradigm, you don't have a lot of messy try/catch code all over, but your still "protected" from exceptions that otherwise interrupt your normal program flow and bubble up to your higher-level exception handling code.

EDIT:

A good article that covers the intended use of exceptions, and why exceptions aren't checked in C#, is the following interview with Anders Heijlsberg, the chief architect of the C# language:

http://www.artima.com/intv/handcuffsP.html

EDIT 2:

To provide a better example that works with the code you posted, perhaps the following will be more useful. I'm guessing at some of the names, and doing things one of the ways I've encountered services implemented...so forgive any license I take:

public PictureDataService: IPictureDataService
{
  public PictureDataService(RepositoryFactory repositoryFactory, LoaderFactory loaderFactory)
  {
     _repositoryFactory = repositoryFactory;
     _loaderFactory = loaderFactory;
  }

  private readonly RepositoryFactory _repositoryFactory;
  private readonly LoaderFactory _loaderFactory;
  private PictureDataRepository _repo;
  private PictureDataLoader _loader;

  public void Save(UserAccount account, UserSubmittedFile file)
  {
    #region Validation
    if (account == null) throw new ArgumentNullException("account");
    if (file == null) throw new ArgumentNullException("file");
    #endregion

    using (PictureDataRepository repo = getRepository())
    using (PictureDataLoader loader = getLoader())
    {
      PictureData pictureData = loader.GetPictureData(file);
      pictureData.For(account);
      repo.Save(pictureData);
    } // Any exceptions cause repo and loader .Dispose() methods 
      // to be called, cleaning up their resources...the exception
      // bubbles up to the client
  }

  private PictureDataRepository getRepository()
  {
    if (_repo == null)
    {
      _repo = _repositoryFactory.GetPictureDataRepository();
    }

    return _repo;
  }

  private PictureDataLoader getLoader()
  {
    if (_loader == null)
    {
        _loader = _loaderFactory.GetPictureDataLoader();
    }

    return _loader;
  }
}

public class PictureDataRepository: IDisposable
{
  public PictureDataRepository(ConnectionFactory connectionFactory)
  {
  }

  private readonly ConnectionFactory _connectionFactory;
  private Connection _connection;

  // ... repository implementation ...

  public void Dispose()
  {
    GC.SuppressFinalize(this);

    _connection.Close();
    _connection = null; // 'detatch' from this object so GC can clean it up faster
  }
}

public class PictureDataLoader: IDisposable
{
  // ... Similar implementation as PictureDataRepository ...
}
jrista
This what I read a lot of and I agree. I employ the same practices for exception handling and freeing resources. I understand you were giving a quick example but for your example, let's assume that this application will expect an occasional null because thats what users do. You want to handle that and return something meaningful because there are few thing as awful as the NullReferenceException. How would you rewrite that to no longer be naive of null inputs while also recognizing that a null input is nothing truly exceptional?
Nick Swarr
Well, my example was obviously contrived. Remember that I said you should never encounter most exceptions...a properly written application would have checked for null, and either returned an error code, or...thrown one of the few acceptable and recoverable exceptions that should be used: ArgumentNullException. Every C# application I write does extensive argument validation and throws one of ArgumentException, ArgumentNullException, or ArgumentOutOfRangeException if the input is invalid. My UI layer handles anything derived from ArgumentException on a case-by-case subsystem-by-subsystem basis.
jrista
+1  A: 

In situations like you have I usually throw a custom exception to the caller. I have a bit of a different view on exceptions maybe than others have: If the method couldn't do what it is intended to do (ie. What the method name says: Create a user account) then it should throw an exception - to me: not doing what you're supposed to do is exceptional.

For the example you posted, I'd have something like:

public UserAccount Create(UserAccount account)
{
    if (_userRepo.UsernameExists(account.Username))
        throw new UserNameAlreadyExistsException("username is already in use.");
    else
        return _userRepo.CreateUserAccount(account);
}

The benefit, for me at least, is that my UI is dumb. I just try/catch any function and messagebox the exception message like:

try
{
    UserAccount newAccount = accountThingy.Create(account);
}
catch (UserNameAlreadyExistsException unaex)
{
    MessageBox.Show(unaex.Message);
    return; // or do whatever here to cancel proceeding
}
catch (SomeOtherCustomException socex)
{
    MessageBox.Show(socex.Message);
    return; // or do whatever here to cancel proceeding
}
// If this is as high up as an exception in the app should bubble up to, 
// I'll catch Exception here too

This is similar in style to a lot of System.IO methods (http://msdn.microsoft.com/en-us/library/d62kzs03.aspx) for an example.

If it becomes a performance problem, then I'll refactor to something else later, but I've never needed to squeeze performance out of a business app because of exceptions.

SnOrfus