try catch bad form?

Question

hi there

i think i sort of know the answer to this, but there are always many ways to do things (some of which are clearly wrong :) )... I have a little recursive function to find an employees manager's id. this is being used in a import script and it may be that the persons immediate manager has left (been disabled) so we need to find that employees (the manager) manager (and so on) so we can assign stuff to them. in case it isnt obvious, the EmployeesToDisable is a generic list of employees that are marked as disabled in this import. I guess what i am really asking, is : is the overhead associated with catching an exception too much of a trade off to make in this instance. and should i be doing this differntly. this does work fine, but feels like it is bad form..

i have code thus:

private Guid getMyEnabledManagersID(OnlineEmployee e)
    {
     Employee manager;
     try
     {
      //see if Employee e's manager is in the disabled list.
      manager = (from emp in EmployeesToDisable where emp.EmployeeID.Equals(e.ManagerID) select emp).Single();
      //yes they are, so need to call this again 
      return getMyEnabledManagersID(manager);
     }
     catch
     {
      return e.ManagerID;
     }
    }

thanks

nat

Answer 1

Leaving aside the recursion, you should perhaps just use SingleOrDefault and test for null. Actually, you probably don't need the full employee object - you can probably suffice with just returning the id (throughout), i.e.

private Guid getMyEnabledManagersID(Guid managerId)
{
    var disabled = (from emp in EmployeesToDisable 
                    where emp.EmployeeID == managerId
                    select (Guid?)emp.ManagerID).SingleOrDefault();
    return disabled == null ? managerId : getMyEnabledManagersID(disabled.Value);
}

Actually, the biggest concern I have with the original form is that it isn't specific to the type of exception; it could be "thread abort", "zombie connection", "deadlock", etc.

Answer 2

You could use FirstOrDefault() instead of Single and handle the returned null value.

Answer 3

Apart from other answers,

Try/Catch are very costly operations, your simple if statement is faster in terms of performance then expecting a catch and then following the logic.

Try/Catch should not be part of business logic, instead they should only be part of error handling.

Answer 4

As others have pointed out never do that. That is a "worst practice". The exception is there to tell you that you have a logic bug in your program. By catching the exception and continuing on, you hide the logic bug.

Only use Single when you know, positively and for sure, that there is exactly one element in the sequence. If there might be other numbers of elements in the list then use First, FirstOrDefault, SingleOrDefault, or write your own sequence operator; it's not hard to do.

The main reasons to not use this worst practice are:

1) As I said, it hides a bug; those exceptions should never be caught because they should never be thrown in a working program. The exceptions are there to help you debug your program, not to control its flow.

2) Using exceptions as control flow like this makes it difficult to debug a program. Debuggers are often configured to stop on any exception, whether it is handled or not. Lots of "expected" exceptions make that harder. Exceptions should never be expected, they should be exceptional; that's why they're called "exceptions".

3) The catch catches everything, including stuff that probably indicates a fatal error that should be reported to the user.