CODEWITHSUNDEEP
c#exception
scottm
scottm

Reputation: 28701

When to handle the exception?

I am writing an app that I can feed tasks with multiple steps. I have some code similar to what's below and I want to know if this is the normal way to handle exceptions. This code will probably never be seen by anyone else, but it could be, so I'd like to know I'm handling the exceptions as anyone would expect.

IEnumerable<Task> Tasks;

foreach(var task in Tasks)
{
 try
 {
   //boiler plate prep for task (loading libraries, connecting, logging start, etc)

   foreach(var step in task.Steps)
   {  
      try
      {
        step.Execute();
      }
      catch(Exception ex)
      {
        LogStepError(step, ex);
        throw;
      }
    }

    //Notify parties task has been completed successfully, log task completion
  }
  catch(Exception ex)
  {
      LogTaskFailure(task);
  }
  finally
  {
    //close connections, etc
  }
}



interface ITaskStep
{
    void Execute()
    {

    }            
}

I also wanted to add that the Task Steps are implementing the ITaskStep interface, so the implementation of Execute is not my own (well it is in this instance, but someone could implement the interface). My code just loads up the library and runs any ITasks and their ITaskSteps.

Upvotes: 3

Views: 393

Answers (6)

Sean
Sean

Reputation: 4470

If step.Execute() is the only thing that's happening in your for loop, the following might be better: (edited in response to clarification)

IEnumerable<Task> Tasks;

foreach(var task in Tasks)
{
   try
   {
     //boiler plate prep for task
   }
   catch(Exception ex)
   {
     LogTaskFailure(task);
     continue;
   }
   foreach(var step in task.Steps)
   {  
      try
      {
        step.Execute();
      }
      catch(Exception ex)
      {
        LogStepError(step, ex);
        LogTaskFailure(task);
        break;
      }
    }
}

class TaskStep
{
    private void Execute()
    {
       //do some stuff, don't catch any exceptions
    }            
}

This way you don't rethrow the exception.

Upvotes: 2

Rob Kennedy
Rob Kennedy

Reputation: 163277

Go ahead and catch exceptions to log their existence. But if you haven't actually resolved the problem that led to the exception being thrown, then please rethrow it for the caller to handle. Don't swallow it up.

Your code above catches a TaskIsBogusException and a PrinterOnFireException and treats them the same way: Log it and keep going with the next task. Doing that with a bogus task is fine because you're done with the task anyway, but if you catch a PrinterOnFireException and don't rethrow it, then by golly, the printer had better not still be on fire.

If you're not going to rethrow, then only catch the specific exception types that your code knows how to handle at that point. Let everything else (either stuff you don't know how to handle, or stuff you never even thought of) propagate up to the next available exception handler.

Upvotes: 7

Freddy
Freddy

Reputation: 3274

You are catching ALL the exceptions which is OK and as you mentioned is the most common approach, but I don't think it is the right one.

I think you should catch specific exceptions. If you do that you could separate them and you will notice that there are some exceptions that you simply can't handled, but there are others that you can. The code will be better and more robust since you will exactly know what is happening on your code.

This is an example:



try
{
     //Your stuff
}
catch(DivideByZeroException ex)
{
   //Could you manage this?
}
catch(NullReferenceException ex)
{
   //Could you manage this one?
}
catch(IOException ex)
{
      //What about this one?
}
finally
{
      //Any cleanup code
}

Upvotes: 2

Bhaskar
Bhaskar

Reputation: 10681

You might want to handle the completion of your task using a return varibale and use Exception on the whole code pience to catch any generic exception.

Example:

try{
foreach(var step in task.Steps)
       {  
            if(!step.Execute()){
                break;
            }

        }
}catch(Exception ex)
{
}

Upvotes: 0

kemiller2002
kemiller2002

Reputation: 115488

I think I might have written it like this, but you method works also. 

foreach(var step in task.Steps)
       {  
          try
          {
            step.Execute();
          }
          catch(Exception ex)
          {
            LogStepError(step, ex);
            LogTaskFailure(task);
            break;
          }
        }

Upvotes: 0

William Edmondson
William Edmondson

Reputation: 3637

I have done something similar numbers of times when I have a list of tasks that I want to complete even if some of them fail. In fact there are times I know there is a good chance one might fail but I don't want to break out of the loop until all steps have completed.

I think it makes a lot of sense.

Upvotes: 0

Related Questions