33

Usually I put all of my Main method code inside of a try/catch block like so:

public static void Main(string[] args)
{
   try
   {
      // code
   }
   catch (Exception e)
   {
      // code
   }
}

I do this just in case any exceptions manage to slip out of the rest of the program logic, thus allowing me to do something about it, such as display it to console, log it to a file, etc. However, I have been told that this is bad practice.

Do you think it is bad practice?

TheBoss
  • 751
  • 3
  • 9
  • 11

5 Answers5

42

Wrapping any piece of code in a try/catch block without a good reason is bad practice.

In the .NET programming model, exceptions should be reserved for truly exceptional cases or conditions. You should only try to catch exceptions that you can actually do something about. Furthermore, you should should hardly ever catch the base System.Exception class (but rather prefer to catch the more specific, derived exception classes you can handle). And should a truly unexpected exception be encountered during the course of your program's execution, you actually should crash.

Obviously the "correct" answer would have to be made on a case-by-case basis, depending on what's going on inside that // code placeholder in your catch block. But if you're asking for a general rule or "best practice", you should always have a specific reason to catch exceptions, not just wrap all of your code in a giant try/catch block as a matter of course without thinking about it.

Note that if you're simply trying to catch any unhandled exceptions that might occur for the purposes of logging or error reporting, you should be using the AppDomain.UnhandledException event. This is a notification-only event, so it doesn't allow you to handle those exceptions, but it is the right place to implement your logging or error reporting functionality after your application has crashed.


EDIT: As I was catching up on my reading of Raymond Chen's excellent blog, "The Old New Thing", I noticed that he had recently published an article on a similar topic. It's specific to COM, rather than the .NET Framework, but the general concepts regarding error handling are equally applicable to both environments. I thought I'd share a couple of gems from the article here, in support of my [apparently quite controversial] opinion.

Historically, COM placed a giant try/except around your server's methods. If your server encountered what would normally be an unhandled exception, the giant try/except would catch it and turn it into the error RPC_E_SERVERFAULT. It then marked the exception as handled, so that the server remained running, thereby "improving robustness by keeping the server running even when it encountered a problem."

Mind you, this was actually a disservice.

The fact that an unhandled exception occurred means that the server was in an unexpected state. By catching the exception and saying, "Don't worry, it's all good," you end up leaving a corrupted server running.

[ . . . ]

Catching all exceptions and letting the process continue running assumes that a server can recover from an unexpected failure. But this is absurd. You already know that the server is unrecoverably toast: It crashed!

Much better is to let the server crash so that the crash dump can be captured at the point of the failure. Now you have a fighting chance of figuring out what's going on.

You can [and should] read the whole article here on his blog: How to turn off the exception handler that COM "helpfully" wraps around your server.

Bill
  • 3,478
  • 23
  • 42
Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574
  • 1
    @gridzbi: Okay? So do it in the `AppDomain.UnhandledException` event. That's what it's *designed* for. Wrapping your code in a try/catch block is a generally *bad* idea, especially for something like this. A truly unhandled exception that you can't do anything about **should** cause your application to crash. Logging is a secondary concern, and a provision has been made for it. – Cody Gray - on strike Jan 28 '11 at 11:33
  • 4
    Never say "never". There are loads of cases where you have to catch System.Exception. E.g., if you're processing an arbitrary plugin call, which may throw anything, but you shoud kill it gracefully if anything fails. Or if you're interpreting an arbitrary code entered by a user, which may throw absolutely anything - in this case you have to show a stack trace to a user and return back to repl. – SK-logic Jan 28 '11 at 12:04
  • @SK-logic: I'm not familiar with instances where the user can raise their own exceptions. And yes, I suppose there might be 3rd party libraries who get things wrong and raise `System.Exception`, but that's still no excuse for swallowing *all* of your code with a try/catch block. Mind you, the rest of my answer is pretty relenting about having to decide this on a case-by-case basis; the way I perceive the original question is as one of "best practice", not "is this ever conceivably justified?" – Cody Gray - on strike Jan 28 '11 at 12:09
  • @Cody Gray, if it is a plugin, not a library, your application can carry on even if the plugin failed badly - just switch this plugin off and complain. Details of the failure are unimportant. Same for the case if there is an embedded scripting language, where a user is allowed to throw exceptions - it's quite a typical scenario. I was answering to your statement that it's not a good practice for ANY bit of a code, but of course I'd agree for Main(). – SK-logic Jan 28 '11 at 12:14
  • -1 I totally disagree with this view. An exception that reaches the end of the main function can only have one outcome: a program crash on the user's face. Anything is better than that. Logging the exception and doing _something_ to apologize to the user, or rethrowing is a standard. If there's any sort of critical shutdown procedure to perform, it should be performed. Trying to analyze the application state that lead to the crash is also a good idea. – Apalala Jan 30 '11 at 00:14
  • 1
    @Apalala: Yes. When you're debugging the application, that's all a fantastic idea. But NOT when you've deployed the application. Here's the problem: *you're going to crash in the user's face either way*. It doesn't make *any sense at all* to wrap the `Main` method in a try/catch block and *re-throw* the error as you suggested. The error already got thrown! It's already reached the highest level. You accomplish *nothing* by rethrowing it. And I've already *explained* how you log the error. There's a method built in for that; no reason for try/catch. – Cody Gray - on strike Jan 30 '11 at 07:17
  • (continued) Also remember that there's not much you can do once your program encounters an unexpected exception that bubbles all the way up to the `Main` method. You can't *fix* whatever caused the exception at that point, so all you can do is ignore it and move on. But the damaged that caused the exception has already occurred. Your program's state is already corrupted. Better to fail fast than to keep trucking along and put the user's data at further risk. One crash means you're just that much more likely to crash again. Ignoring the inevitable is bad design now matter how you look at it. – Cody Gray - on strike Jan 30 '11 at 07:18
  • @Cody Gray Those are all good arguments. It would be good if you edited the article to explain them, but these comments are good enough. -(-1) – Apalala Jan 30 '11 at 15:53
  • There's something else. On that last catch, you could forget about the crashed application, and launch something new that is gentle with the user (even if the user is another machine). BUT, as @Cody says, an unexpected exception means that the running program doesn't have a clue about what's going on (a bug in a desktop widget may have used up all the memory, as I've witnessed). It's better to die fast and safely. +1 – Apalala Jan 30 '11 at 15:58
  • For the record, yesterday I was studying the source code for several versions of timsort, and it reminded me of program states that some of us like to call _"Can't happen"_. The code for the timsort implementations is full of assertions about the apparently obvious. I won't argument here in favor of leaving or removing assertions on deployed code, but I'll agree with @Cody Gray in that if something like an `AssertionFailed` exception reaches the entry point, it means that the program has absolutely no clue about what's going on, so it better let itself die in peace. – Apalala Jan 30 '11 at 16:08
6

If you're doing the most intelligent thing you can with the error, it's good practice. That's what try/catch is for.

If you're just throwing the error away (or logging it and throwing it away) — especially if you're doing so regardless of the type of exception — that's considered bad practice.

This might raise the question: what if the most intelligent thing is to log it and throw it away? I want to say that would be an exceptional case. But in practice, my code would assert otherwise. I guess I indulge in a lot of bad practice.

harpo
  • 41,820
  • 13
  • 96
  • 131
  • I would agree, except that you still should never catch `System.Exception`. You would catch a more specific, derived exception that you *could* conceivably do something about. – Cody Gray - on strike Jan 28 '11 at 11:24
4

I aggree with 90% of what Cody says. There are some situations similar to the pluggin example where you may want to catch system exceptions. Here is another example consider consuming a WCF web service.

Goal: Consume the service and dispose even if an error is encountered. Allow the error to bubble.

public static bool DoRemoteWebServiceWork()
{
    bool result;
    RemoteWebServiceClient client = new RemoteWebServiceClient();
    try
    {
        result = client.DoWork();
        client.Close();
    }
    catch (Exception)
    {
        client.Abort(); //dispose
        throw;//This service is critical to application function. The application should break if an exception is thrown.
        //could log end point and binding exceptions to avoid ignoring changes to the remote service that require updates to our code.
    }
    return result;
}

Goal: Consume the service and dispose even if an error is encountered. Prevent the error from bubbling.

public static bool DoRemoteWebServiceWork()
{
    bool result;
    RemoteWebServiceClient client = new RemoteWebServiceClient();
    try
    {
        result = client.DoWork();
        client.Close();
    }
    catch (Exception)
    {
        client.Abort(); //dispose
        //throw; //This service is auxiliary to the applications primary function. We have no influence over the service and therefore cannot fix it.
        //could log end point and binding exceptions to avoid ignoring changes to the remote service that require updates to our code.
    }
    return result;
}
Nathaniel
  • 137
  • 1
  • 2
1

That depends on what you do if you catch an error. If you just catch all errors to appear to handle them gracefully - bad practice. I'd say it is bad practice even if you only log there - log locally.

If you really do something to recover the error (e.g. you threw it yourself and know what to do about it) - I'd vote OK :)

Leonidas
  • 2,440
  • 15
  • 22
1

Definetly, yes, it is a bad practice, to use Exception class

You should care about types of an exception, not cutting all exceptions on catch block, preventing them to inform system of the error.

Exception class is a base class, and, is a top-level exception can be catched in code. Use the most specific Exceptions in catch block, specifically, the only that, which you code raises in a try {...} catch{...} block, and only if it can be effectively resolved on that particular level (in a function, where try.. catch block is declared)

Alan Turing
  • 2,482
  • 17
  • 20