views:

254

answers:

8

Working on a project where a sequential set of methods must be run every x seconds. Right now I have the methods contained within another "parent method", and just sequentially call them right after another.

class DoTheseThings()
{
    DoThis();
    NowDoThat();
    NowDoThis();
    MoreWork();
    AndImSpent();
}

Each method must run successfully without throwing an exception before the next step can be done. So now I wrapped each of those methods with a while and try try catch, then in the catch re execute that method again.

while( !hadError )
{
    try
    {
         DoThis();
    }
    catch(Exception doThisException )
    {
         hadError = true;
    }

}

This seems smelly and not very dry. Is there a better way to do this so I'm not wrapping any new functionality in the same methods. Isn't some kind of Delegate collection the proper way to implement this?

Is there a more "proper" solution?

A: 

What would be the reason that an error was occuring?

If this were a resource issue, such as access to something like a connection or object, then you might want to look at using monitors, semaphores, or just locking.

lock (resource) 
{
    Dosomething(resource);
}

This way if a previous method is accessing the resource, then you can wait until it releases the resource to continue.

Ideally, you shouldn't have to run a loop to execute something each time it fails. It is failing at all, you would want to know about the issue and fix it. Having a loop to always just keep trying is not the right way to go here.

nyxtom
+5  A: 
Action[] work=new Action[]{new Action(DoThis),   new Action(NowDoThat),    
    new Action(NowDoThis),    new Action(MoreWork),    new Action(AndImSpent)};
int current =0;
while(current!=work.Length)
{
   try
   {
      work[current]();
      current++;
   }
   catch(Exception ex)
   {
      // log the error or whatever
      // maybe sleep a while to not kill the processors if a successful execution depends on time elapsed  
   }
}
Ovidiu Pacurar
Any reason not to just use the builtin Action delegate?
Mark Brackett
If you find the need to comment "SimpleDelegate," that should definitely be the name itself.
Aidan Ryan
And it needs to rerun the action if it does fail.
nyxtom
It does rerun because the current++ does not execute if an exception occurs in work[current]()
Ovidiu Pacurar
Mark is right. I will edit the answer.
Ovidiu Pacurar
+1  A: 

Isn't some kind of Delegate collection the proper way to implement this?

Delegate is a possible way to solve this problem.

Just create a delegate something like:

public delegate void WorkDelegate();

and put them in arraylist which you can iterate over.

Dave Hillier
A: 

I'd do what Ovidiu Pacurar suggests, only I'd use a foreach loop and leave dealing with array indexes up to the compiler.

Robert Rossney
+1  A: 

your example seems ok.. its a dry one but will do the job well!! actually if this methods execute db access.. you can use transaction to ensure integrity...

if your dealing with shared variables for multi threader programs.. it is cleaner to use synchronization.. the most important thing in coding is that you write the proper code... that has less bugs.. and will do the task correctly..

Aristotle Ucab
A: 

Simple delegate approach:

Action<Action> tryForever = (action) => { 
   bool success;
   do {
     try {
       action();
       success = true;
     } catch (Exception) {
       // should probably log or something here...
     }
   } while (!success);
};

void DoEverything() {
   tryForever(DoThis);
   tryForever(NowDoThat);
   tryForever(NowDoThis);
   tryForever(MoreWork);
   tryForever(AndImSpent);
}

Stack approach:

void DoEverything() {
  Stack<Action> thingsToDo = new Stack<Action>(
    new Action[] { 
      DoThis, NowDoThat, NowDoThis, MoreWork, AndImSpent 
    }
  );

  Action action;
  while ((action = thingsToDo.Pop()) != null) {
     bool success;
     do {
       try {
         action();
         success = true;
       } catch (Exception) {
       }
     } while (!success);
  }
Mark Brackett
+1  A: 
public void DoTheseThings()
{
    SafelyDoEach( new Action[]{
        DoThis,
        NowDoThat,
        NowDoThis,
        MoreWork,
        AndImSpent
    })
}

public void SafelyDoEach( params Action[] actions )
{
    try
    {
        foreach( var a in actions )
            a();
    }
    catch( Exception doThisException )
    {
        // blindly swallowing every exception like this is a terrible idea
        // you should really only be swallowing a specific MyAbortedException type
        return;
    }
}
Orion Edwards
Doesn't repeat the failed action.
Mark Brackett
+2  A: 

I have a personal religious belief that you shouldn't catch System.Exception, or more accurately, you should only catch the exceptions you know how to handle.

That being said, I am going to assume that each one of the methods that you are calling are doing something different, and could result in different exceptions being thrown. Which means you would likely need to have different handlers for each method.

If you follow my religion as well, and the second statement is true, then you are not repeating code unnecessarily. Unless you have other requirements, my recommendations to improve your code would be:

1) Put the try-catch in each method, not around each method call.

2) Have the catches within each method catch ONLY the exceptions you know about.

http://blogs.msdn.com/fxcop/archive/2006/06/14/631923.aspx http://blogs.msdn.com/oldnewthing/archive/2005/01/14/352949.aspx http://www.joelonsoftware.com/articles/Wrong.html

HTH ...

Or, still use the delegate approach, but also pass in a Predicate<Exception> to see if it should be caught or thrown.
Mark Brackett