6

.NET Programming guidelines state that we should not catch general exceptions. I assume the following code is not very good because of the general exception type catch:

    private object CreateObject(string classname)
    {
        object obj = null;
        if (!string.IsNullOrEmpty(classname))
        {
           try
           {
              System.Type oType = System.Type.GetTypeFromProgID(customClass);
              obj = System.Activator.CreateInstance(oType);
           }
           catch (Exception ex)
           {
               Log.Error("Unable to create instance for COM Object with class name " + classname + "\n" + ex.Message);
           }
        }
        return obj;
    }

In the following code I catch particular exceptions but not all of them and then I re-throw the exception in case is different from the non-generic exceptions. However the function "CreateInstance" can throw many exceptions (ArgumentNullException, ArgumentException, NotSupportedException, TargetInvocationException, MethodAccessException, MemberAccessException, InvalidComObjectException, MissingMethodException, COMException, TypeLoadException).

Is it acceptable to catch all other individual exceptions? Or is there a better way?

    private object CreateObject(string classname)
    {
        object obj = null;
        if (!string.IsNullOrEmpty(classname))
        {
           try
           {
              System.Type oType = System.Type.GetTypeFromProgID(customClass);
              obj = System.Activator.CreateInstance(oType);
           }
           catch (NotSupportedException ex)
           {
              Log.Error("...." + ex.Message);
           }
           catch (TargetInvocationException ex)
           {
              Log.Error("...." + ex.Message);
           }
           catch (COMException ex)
           {
              Log.Error("...." + ex.Message);
           }
           catch (TypeLoadException ex)
           {
              Log.Error("...." + ex.Message);
           }
           catch (InvalidComObjectException ex)
           {
              Log.Error("...." + ex.Message);
           }
           catch (Exception ex)
           {
               Log.Error("Unable to create instance for COM Object with class name " + classname + "\n" + ex.Message);
               throw;
           }
        }
        return obj;
    }
David Basarab
  • 72,212
  • 42
  • 129
  • 156
Ioannis
  • 2,985
  • 4
  • 25
  • 19
  • Duplicate of: http://stackoverflow.com/questions/204814/is-there-any-valid-reason-to-ever-ignore-a-caught-exception –  Sep 21 '09 at 12:31

7 Answers7

12

As a general rule you shouldn't catch exceptions unless:

  1. You have a specific exception that you can handle and do something about. However in this case you should always check whether you shouldn't be trying to account for and avoid the exception in the first place.

  2. You are at the top level of an application (for instance the UI) and do not want the default behaviour to be presented to the user. For instance you might want an error dialog with a "please send us your logs" style message.

  3. You re-throw the exception after dealing with it somehow, for instance if you roll back a DB transaction.

In this example why are you catching all these different types? It seems to me that your code can just be:

try
{
    System.Type oType = System.Type.GetTypeFromProgID(customClass);
    return System.Activator.CreateInstance(oType);
}
catch (Exception ex)
{
    Log.Error("...." + ex.Message);

    //the generic catch is always fine if you then do this:
    throw;
}

So your problem is an example of rule (3) - you want to log an exception, but then continue and throw it on up.

All the different types are there so that in certain cases that you know you can handle (i.e. case 1). For instance suppose that you know that there is an unmanaged call that works around COMException - then your code becomes:

try
{
    System.Type oType = System.Type.GetTypeFromProgID(customClass);
    return System.Activator.CreateInstance(oType);
}
catch (COMException cex)
{   //deal with special case:
    return LoadUnmanaged();
}
catch (Exception ex)
{
    Log.Error("...." + ex.Message);

    //the generic catch is always fine if you then do this:
    throw;
}
Keith
  • 150,284
  • 78
  • 298
  • 434
  • I catch all these different types of exceptions because they can happen (e.g. use can pass a class name that is not supported or not registered properly) and in that case there is no a good way to check in advance for an exception (as I can check for ArgumentNullException). – Ioannis Sep 21 '09 at 12:54
  • I'm sure that they can all happen, but unless you're going to do something specific for that exception type you're better off just catching the general `Exception` and throwing it on up. – Keith Sep 21 '09 at 21:29
  • 1
    This answer is exactly right. The key is that you're throwing the general exception upwards not swallowing them. – justin.m.chase Dec 21 '10 at 17:58
9

It's quite acceptable to catch general exceptions in .NET 2+ of the framework.

-- Edit

The only reason you wouldn't, is if you can do something different with a different exception. If you plan on handling them all the same, just catch the general (or specific one you are after, and let anything else go up).

Noon Silk
  • 54,084
  • 6
  • 88
  • 105
  • 3
    I agree. Sometimes I feel we take this not catching general exception to extremes. – David Basarab Sep 21 '09 at 12:29
  • 6
    I disagree. This will leave your application in an exceptional state. You should only handle exceptions you can do something about. Simply logging and moving on is a bad idea. – justin.m.chase Dec 21 '10 at 17:55
  • @justin.m.chase: That's a very odd position. So, anytime your app crashes, you'd prefer to have your users see the .net general error page instead of your own? – Noon Silk Dec 21 '10 at 21:34
  • 1
    Logging and moving on is a bad idea for unknown exceptions, because you don't know how bad the exception is. For example, what happens if you hit an OutOfMemoryException? You shouldn't just log that error and move on. The program should stop running. It it better to "fail fast" than to try to keep an unstable program running. http://www.martinfowler.com/ieeeSoftware/failFast.pdf – Phil Feb 07 '11 at 17:05
  • @Phil: You didn't understand my response. I suggest you take greater care in the future. – Noon Silk Feb 07 '11 at 21:10
  • The impression I'm getting is that you're saying it's ok to catch a general System.Exception, handle it, and continue execution. Correct? – Phil Feb 07 '11 at 21:30
  • @Phil: No, not correct. He was wondering if there is some specific reason not to catch general exceptions. I'm saying, it's fine, if you can do something with it. That is, log, or exit gracefully, or whatever you consider appropriate. I'm not saying you catch exceptions and ignore them, letting your application continue onward as if nothing went wrong. Catching too many specific exceptions is wrong. Catching some specific exceptions is clearly correct (say, FileNotFound, whatever else you can imagine). – Noon Silk Feb 07 '11 at 21:48
  • 2
    @Noon I would agree with that. However "If you plan on handling them all the same, just catch the general" could be interpreted by an inexperienced programmer that it's ok to catch a general System.Exception, do something, and then continue execution. It might be good to specify that if you don't know what type of error you are handling, you better rethrow it or exit. – Phil Feb 08 '11 at 17:29
  • Topic starter exactly does not do same thing. if he puts general catch it should have if(ex is ... or ex is ...){} else {throw;} – Evgeny Gorbovoy Nov 25 '20 at 17:20
1

I think it is okay to catch all exceptions to intercept a problem and display a friendly message to a user rather than some scary stack output.

For as long as you don't just swallow exceptions, but log them and react to them appropriately in the background.

1

If you want to do something special with a different type of exception then catch it in a separate catch block. Otherwise it's meaningless to log them separately.

Canavar
  • 47,715
  • 17
  • 91
  • 122
1

There is not a rigid rule that we should not use general exception but guideline says that whenever we have a option to handle a specific type of exception we should not go for generic exception..

If we are sure that all the exception will be handled in same way then use generic exception otherwise go for each and every specific exception and generic should come in last for the some unknown exception..

and sometimes in your application any exception occurs that is not handled in the code due to specific exception handling then your application may go for crash..

so the better approach is handle all specific exception and then give a corner to generic exception also so that application remain stable without crash..

and those unwanted generic exception can be reported or logged somewhere for future version improvement of application..

Jaswant Agarwal
  • 4,755
  • 9
  • 39
  • 49
1

It's considered poor practise to habitually catch the base Exception when dealing with handling errors, because it shows a potential lack of understanding as to what you are actually handling. When you see a block of code catch Exception what it reads is, "if anything goes wrong here, do this", where that anything could range from a NullReferenceException to an OutOfMemoryException.

The danger in treating all errors the same, is it implies you don't care about how severe the error might be or how you might go about resolving the error. 99% of the time, when I see the code catch(Exception ex), it's immediately followed by code which swallows the exception, giving no clue to actually why the statements failed. Ugh.

Your example of error logging shows the right way to use the base Exception, in a situation when you genuinely want to treat all exceptions the same, usually at the top-level of an application to prevent the application terminating in an ugly mess.

Paul Turner
  • 38,949
  • 15
  • 102
  • 166
1

I agree with Keith's answer. The real issue with both code examples is that they can return null. You have a method called CreateObject that returns an instance of type object. That method failed for some reason, such as a COMException. Both code examples will then return null. The calling code is now responsible for checking the result vs. null, or it will throw a NullReferenceException.

Quoting Framework Design Guidelines 2nd ed.:

If a member cannot successfully do what it is designed to do, it should be considered an execution failure, and an exception should be thrown.

It didn't do what its name suggests. Throw!

Keith's code is fine; it's OK to log the exception and rethrow.

This is a private method, so arguably the design guidelines don't apply. I'd still apply them here, though - why litter your code with "if (result == null)" when you can use exception handling instead?

TrueWill
  • 25,132
  • 10
  • 101
  • 150
  • It is sometimes useful to have a method which will try to create an object, but return null if it cannot; it is often a good idea, however, to give the method a name which suggests that's what it will do. Microsoft prefers to have such routines return a boolean for success/failure and have as a byref parameter an object reference variable to store the new object, but I prefer returning the value or null, since it allows one to define a new variable in the statement that attempts to create the instance. – supercat Mar 01 '11 at 19:08
  • @supercat: Which would be fine if we could trust other developers (and ourselves) to always check if the returned value is null... – TrueWill Mar 01 '11 at 20:54
  • If call a function called TryGetFoo without considering that it might fail, they deserve what they get. They could even more easily ignore the return value (success/failure flag) from an MS-pattern "TryGetFoo" method. The MS-pattern version does have a feature in some situations which may be either a good or bad thing: it's possible for a function to throw an exception but still set the passed-in variable to something before it does so. This may be good if the variable is an IDisposable, but could cause confusion if the return code of the create method is ignored. – supercat Mar 01 '11 at 22:28