Using a message class static method taking in an action to wrap Try/Catch

Question

I have a Result object that lets me pass around a List of event messages and I can check whether an action was successful or not.

I've realized I've written this code in alot of places

Result result;

try
{
    //Do Something 
    ...

    //New result is automatically a success for not having any errors in it
    result = new Result(); 
}
catch (Exception exception)
{
    //Extension method that returns a Result from the exception
    result = exception.ToResult();
}

if(result.Success) ....

What I'm considering is replacing this usage with

public static Result CatchException(Action action)
{
    try
    {
        action();
        return new Result();
    }
    catch (Exception exception)
    {
        return exception.ToResult();
    }
}

And then use it like

var result = Result.CatchException(() => _model.Save(something));

Does anyone feel there's anything wrong with this or that I'm trading reusability for obscurity?

Edit: The reason I am trapping all exceptions is I use this code inside of my ViewPresenter classes at any point I interact with my model since if I get an unhandled exception I'd rather display the actual error to the user (internal app) as opposed to just redirecting them the generic error page.

Answer 1

I don't think there is anything wrong with using a functional approach, and passing in a lambda. That is perfectly valid.

That being said, I'd question your use in this specific scenario. Trapping all exceptions into a general purpose "Result" class seems dangerous. Ideally, you should be catching exceptions which you can handle correctly - not every possible exception and continuing.

That being said, if you really want to do this, I have a couple of suggestions for you:

1) I'd make a static, immutable result for success, and just do:

result = result.Success;

This avoids generating a new "success" each time you succeed, which hopefully is the most common option.

2) I'd probably avoid the extension method, and do:

result = new Result(exception);

This will be much more clear and obvious in intent, and there's really no reason to do an extension method in this case...

Answer 2

You shouldn't be catching Exception like this. You should only be catching known exception types that you know how to handle.

If you did that, you code would no longer be so repetitive, so there would be no case for using this pattern for re-usability.

All round this looks like a very bad idea, not just the lambda pattern, I'm talking about the handling of exceptions by converting them into results objects. You are basically allowing yourself to ignore all exceptions and continue working. There is a reason why C# error handling is done with exceptions and not with return codes, and that is because exceptions allow for structured, hierarchical and call stack based handling. Unless you have simplified the code for this question, I would reconsider your exception handling process.

---

In general though, I don't see anything wrong with the lambda pattern. I use something similar myself for wrapping multiple retries of a code block. But in this case I don't think it's what you need.

Answer 3

If you feel the need to write such an automated exception handler system you may be over-using exceptions, which could have serious performance implications.

However, this appears to be a generic system for converting exceptions into error codes - so why not just use error codes in the first place? Or learn to use exceptions more effectively so you don't feel the need to convert them into error codes? The fact that you are trying to convert them to something else should raise alarms and suggest to you that you're missing the point somewhere.