Removing excessive try-catch blocks

Question

I'm refactoring a medium-sized WinForms application written by other developers and almost every method of every class is surrounded by a try-catch block. 99% of the time these catch blocks only log exceptions or cleanup resources and return error status.

I think it is obvious that this application lacks proper exception-handling mechanism and I'm planning to remove most try-catch blocks.

Is there any downside of doing so? How would you do this? I'm planning to:

• To log exceptions appropriately and prevent them from propagating to the user, have an Application.ThreadException handler

• For cases where there's a resource that needs cleanup, leave the try-catch block as it is

Update: Using using or try-finally blocks is a better way. Thanks for the responses.

• In methods that "return-false-on-error", let the exception propagate and catch it in the caller instead

Any corrections/suggestions are welcome.

Edit: In the 3rd item, with "return-false-on-error" I meant methods like this:

bool MethodThatDoesSomething() {
    try {
       DoSomething(); // might throw IOException
    } catch(Exception e) {
       return false;
    }
}

I'd like to rewrite this as:

void MethodThatDoesSomething() {
   DoSomething(); // might throw IOException
}

// try-catch in the caller instead of checking MethodThatDoesSomething's return value
try {
   MethodThatDoesSomething()
} catch(IOException e) {
   HandleException(e);
}

Answer 1

The best is as said by others, do exception handling at 1 place. Its bad practice to conceal the raised exception rather than allowing to bubble up.

Answer 2

For cleanup rather use try-finally or implement the IDisposable as suggested by Amittai. For methods that return bool on error rather try and return false if the condition is not met. Example.

bool ReturnFalseExample() {
    try {
        if (1 == 2) thow new InvalidArgumentException("1");
    }catch(Exception e) {
       //Log exception  
       return false;
    }

Rather change to this.

bool ReturnFalseExample() {
    if (1 == 2) {
       //Log 1 != 2
       return false;
    }

If i'm not mistaken try catches are an expensive process and when possible you should try determine if condition is not met rather then just catching exceptions. }

Answer 3

As an option for "return-false-on-error" you can clean up the code this way:

    static class ErrorsHelper {
        public static bool ErrorToBool(Action action) {
            try {
                action();
                return true;
            } catch (Exception ex) {
                LogException(ex);

                return false;
            }
        }

        private static void LogException(Exception ex) {
            throw new NotImplementedException();
        }
    }

and usage example:

    static void Main(string[] args) {
        if (!ErrorToBool(Method)) {
            Console.WriteLine("failed");
        } else if (!ErrorToBool(() => Method2(2))) {
            Console.WriteLine("failed");
        }
    }

    static void Method() {}

    static void Method2(int agr) {}

Answer 4

You may try to use AOP.

In AOP through PostSharp, for example, you can handle exceptions in one central place (piece of code) as an aspect.

Look at the examples in documentation to have an idea => Docs on Exception Handling with PostSharp.

Answer 5

You should only handle only the exceptions that you are expecting, know how to handle and they are not corrupt the state of your application, otherwise let them throw.

A good approach to follow is to log the exception first, then Restart your application, just like what Microsoft did when office or visual studio crashing. To do so you have to handle the application domain unhanded exception, so:

AppDomain.CurrentDomain.UnhandledException += OnCurrentDomainUnhandledException;

//Add these two lines if you are using winforms
Application.ThreadException += OnApplicationThreadException;
Application.SetUnhandledExceptionMode(UnhandledExceptionMode.CatchException);

private void OnCurrentDomainUnhandledException(object sender, System.Threading.ThreadExceptionEventArgs e)
{
    //Log error

    //RestartTheApplication
}

Here an example on how to restart your application.

Answer 6

we can remove try and catch by adding condition Like

 string emailAddresses = @"^([\w\.\-]+)@([\w\-]+)((\.(\w){2,3})+)$";
        if (!Regex.IsMatch(Email, emailAddresses))
        {
            throw new UserFriendlyException($"E-mail Address Is not Valid");
        }**strong text**