views:

422

answers:

7

.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;
    }
+12  A: 

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
I agree. Sometimes I feel we take this not catching general exception to extremes.
David Basarab
+1  A: 

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.

Developer Art
+1  A: 

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
+1  A: 

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
+7  A: 

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
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
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
+1  A: 

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.

Programming Hero
+1  A: 

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