catch()-block as flow-direction (kind of)

Question

I'm currently using such blocks of code in my project

     public ActionStatus GetStuff(out output)
     {
        try
        {
            if(!validation)
              return ActionStatus.DataInvalid;
            //do possible error stuff here
            output = data;
            return ActionStatus.Successful;
        }
        catch
        {
            output = null;
            return ActionStatus.Failed;
        }
     }

I use this, as the ActionStatus enum is enough for the program to continue (these things are at most times reading data from Cache or Database), as the enum provides enough info to provide a message to the user. Those methods will be having a PostSharp-Weaver attribute, to log the message for later debug use.

In my opinion, this is an OK-ish way of handling such non-critical exceptions, as the enum is enough to continue; on the other hand, I always have the saying "don't control your program flow with exceptions", but I interpret that as using

if(ex == fooException)
  doThis();
else if (ex == barException)
  doThat();

What is your opinion on that?

UPDATE: Off the two answers I can see, that there is need for extra clarification, specificly about the PostSharp-weaver.

PostSharp is an Aspect Orientated Programming (AOP) framework, it basicly allows you to create classes, that overwrite specific methods, like OnEnter, OnExit, OnException, etc. This allows you to keep specific (e.g. logging) code at a minimum, you just have to put an attribute to the method or class you want to modify at compiletime. By that, I won't be losing any Exception informations, as the callstack and everything else will still be the same.

UPDATE 2: missed an assignment, out parameters have to be assigned.

Answer 1

Update Based on OP's Update (AOP framework)

You have no idea how happy I am to discover that the framework you use ensures that you're not losing any of the original exception information. That makes me feel somewhat better about your design.

However, in that scenario, I fall back to @silky's advice from the comment he left here, and the cautionary note from @mfx on the question itself: Why bother catching at all? Your software design relies on higher levels checking a return value from your method. That's very easy for developers to miss, or forget to do. At least if the exception bubbles up the stack, and occurs, it can be noted in testing and the code tweaked. If the return value isn't checked, the bugs can become considerably more insidious.

Again, I point you to the developer framework guidelines from Microsoft on exception handling. (In the annotated book form -- which I highly recommend -- there are a lot of notes on why design decisions were made, including notes about why they decided to -- usually -- shy away from return codes in favor of exceptions.)

Regarding your (updated) code, why would this not be an acceptable redesign?

 public WhateverTypeOutputWas GetStuff()
 {
    if(!validation)
        return ActionStatus.DataInvalid;
    //do possible error stuff here
    return data;
 }

What we have here is the exception bubbling up the stack, for higher levels to deal with, without the possibility of ignoring the return code.

The way the code was written in the question, it sounds like the developer will get a NullReferenceException if he forgets to check your status code. In that case, why is the status code needed at all? Can the developer safely assume that a non-null return of whatever output was meant a successful run? And with the design above, we cut out half the method, bubble up exceptions correctly, use return values correctly (as per Microsoft's framework design guidelines) and simplify life for the developer consuming this method. Win-win.

Original Answer

Why would you strip away the valuable information contained in the exception you catch? Rethrow it:

catch(Exception ex)
{
    // Any handling (like logging, etc.) here
    // If no handling, don't bother catching
    throw;
}

and allow the higher layer of code to catch and determine what to display to the user (if anything) and (hopefully) log the detailed information in case it's needed.

There are occasions in which it'd be OK to do something like this, but in my experience they've been few and far between -- it usually has to be a very non-critical piece of code for me to simply discard the exception altogether, and like it or not, that's what's happening here.

Regarding exceptions as control flow, I don't think that the following (from your original question) would be considered that:

if(ex == fooException)
  doThis();
else if (ex == barException)
  doThat();

I think the design guidelines regarding exceptions as control flow actually refer to throwing exceptions as a way to get out of loops early (instead of using break, EG), using exceptions when nothing went wrong in the code as simple early returns, etc.

I point you to Microsoft's official Design Guidelines for Exceptions; as that is what I attempt to follow in the course of my work, and I expect developers working with me to do the same. (That is, unless we have a really good, documented reason not to. And again, those are rare.)

Remember that exceptions are for exceptional events in your code -- typically, for example, error conditions. There's a reason we aren't expected to check success/failure codes in the .NET base class library -- the preferred model in .NET is to throw an exception if the method failed to perform. Thus error-handling is relegated to try/catch and return codes are used to return useful data to the client code consuming your methods.

Answer 2

I think your model is quite reasonable.

And the statement 'don't control your program flow with exceptions' is a little weird, because that's exactly what you should be doing. If you just carry on as if the exception never happened then I can't imagine what you are doing! :)

And I also agree, your 'if' statement suggestion there is quite ridiculous, and anyone doing that should be shot.

But, of course, something like

try {
}
catch(SomeException e) { }
catch(Exception e){ }

Is reasonable.

-- Edit:

Update to confirm I see no bearing that your AOP has on doing this; it's quite reasonable, IMHO, regardless.