Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created this class which supposed to take a method and try to execute it. If it fails lets say 10 times, it will throw the original exception thrown by the delegate.

Do you guys have any ideas for improvement about that code?

I am particularly interested, in where to put this code. Right now it is within a "FileHandler" Assembly because the code was used for serialization. But since it can be used generally I want to move it to a different assembly. Unfortunately I did not come up with a better name (other than "Utility" or "Helper"). Unfortunately I did not come with a really meaningful name...maybe something related to "invoke" or so?

public static class FileHandler
{
  private const int MAX_TRIES = 10;
  private const int WAIT_FOR_RETRY_DELAY = 250;

  public static T ExecuteFuncWithRetry<T>(Func<T> func, string errorMessage)
  {
    var retryFunc = new FuncWithRetry<T>(errorMessage, func);
    var ret = retryFunc.PerformAction();
    return ret;
  }

  private class FuncWithRetry<T>
  {
    private readonly string _errorMessage;

    private readonly Func<T> _func;

    public FuncWithRetry(string errorMessage, Func<T> func)
    {
      _errorMessage = errorMessage;
      _func = func;
    }

    public T PerformAction()
    {
      var tries = 1;
      Exception exception = null;
      do
      {
        try
        {
          var retval = _func();
          return retval;
        }
        catch (Exception e)
        {
          exception = e;
          Thread.Sleep(WAIT_FOR_RETRY_DELAY);
        }
        finally
        {
          tries++;
        }
      } while (tries <= MAX_TRIES);

      throw new Exception(_errorMessage, exception);
    }
  }
}
share|improve this question
    
@derape side note - I don't think creating assembly for single class is a good idea –  Sergey Berezovskiy Jun 11 at 12:53
    
@SergeyBerezovskiy I know, but lets assume I do not have suiting assemblies, where would you put it, or what should the assembly name tell me? Where would I look for something like that... –  derape Jun 11 at 12:54
    
This doesn't seem to be complete code, as some variables (such as MAX_TRIES are never declared). I assume you're just missing a couple of const declarations, but it's still helpful to have them in there for completeness –  Ben Aaronson Jun 11 at 13:33
    
@BenAaronson you are right, here you go =) –  derape Jun 11 at 13:37
2  
Use a concrete exception class, not throw Exception. I'd consider wrapping all exceptions (each attempt might have returned a different one) in an AggregateException. –  CodesInChaos Jun 11 at 13:51

1 Answer 1

up vote 4 down vote accepted

Assemblies and naming

Assemblies are units of deployment -- you should put this code in the assembly it's being used in; don't over-think it. Use namespaces, not assemblies, for logical partitioning of code, and don't create a new assembly just because some code may be used elsewhere. Wait until it is used elsewhere.

However

However, I would question the usefulness of such a function.

You catch an Exception, which is generally not recommended -- what happens if it's an OutOfMemoryException? But you're forced to catch an Exception as you have no way of knowing which exceptions could be thrown.

You throw an Exception, forcing callers in turn to catch an Exception. And only the last exception is kept around (in the inner exception) -- what if some exceptions require special handling, or logging? All the information contained in them is lost. As @CodesInChaos commented, an AggregateException would be better.

What if it's being called on the UI thread, and it wouldn't make sense to call Thread.Sleep between retries?

The problem with this method is that it tries to be too general. Retry logic will often have very specific requirements that can't be abstracted away into one function. For instance, say I'm getting exceptions that contain HTTP status codes. If I'm getting 503 Service Unavailable, I may want to try again with exponential back-off. But if it's a 400 Bad Request, there is something wrong with my code and I should log it without retrying.

share|improve this answer
    
What about having a retrier class with a template method pattern? The assumptions made in the OP won't always be good as you've pointed out, but they would probably make for a decent "default" implementation –  Ben Aaronson Jun 11 at 14:09
    
Like the AggregateException. Actually I changed the implementation to be more context specific as you proposed. Thank you –  derape Jun 13 at 10:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.