Multiple methods that need to be handled the same way

Question

Hello,

In a piece of C# that I am writing at the moment I need to handle several methods with the same signature in the same way. Also there might be more of these methods in the future. Instead of repeating the same kind of logic over and over I thought up the following:

private delegate bool cleanStep(BuildData bd, out String strFailure);

List<cleanStep> steps = new List<cleanStep>();
steps.Add(WriteReadme);
steps.Add(DeleteFiles);
steps.Add(TFSHelper.DeleteLabel);
steps.Add(TFSHelper.DeleteBuild);

List<cleanStep>.Enumerator enumerator = steps.GetEnumerator();
bool result = true;
while (result && enumerator.MoveNext())
{
   result = enumerator.Current.Invoke(build, out strFailure);
   if (!result)
   {
      logger.Write(LogTypes.Error, strFailure);
   }
}

I think this has some nice features but it also feels a bit over enginered and obfuscating.

Can you thank of a better a way of doing this ?

btw:

• it doesn't needs to be transactional.
• strFailure does not hide exceptions it wraps them completely when necessary

Thanks.

Answer 1

Why not use a foreach loop and just break? (I've renamed cleanStep to CleanStep here for conventionality - I suggest you do the same.)

foreach(CleanStep step in steps)
{
    string failureText;
    if (!step(build, out failureText))
    {
        logger.Write(LogTypes.Error, strFailure);
        break;
    }
}

Note that this also obeys the contract of IEnumerator<T> where your current code doesn't - foreach automatically calls Dispose, and IEnumerator<T> implements IDisposable. It won't be an issue in this case, but with iterator blocks, disposal is used to execute finally blocks.

Answer 2

Your solution is both straight foward and easy to understand. I can see no reason to do it another way :)

The only thing I'd suggest is to replace your iterator with a foreach loop and break on an error.

Answer 3

Re obfuscated - well foreach with break might be clearer (plus it'll Dispose() the enumerator, which you aren't doing).

Actually, a "params cleanStep[] targets" might help:

static bool RunTargets(params cleanStep[] targets)
{
    // detail as per Jon's post
}

then you can call:

bool foo = RunTargets(WriteReadme, DeleteFiles,
              TFSHelper.DeleteLabel, TFSHelper.DeleteBuild);

Answer 4

I would return an Exception object instead of a string. Since exceptions often have a global policy, I would write some Exception extensions. Now you get:

static Exception Run( this IEnumerable<Step> steps) {
   return 
       steps
       .FirstOrDefault( (step) => step( build ) != null )
       .LogIfFailure();  //or .ThrowIfFailure()
}

The extensions:

public static class ExceptionExtensions {
    private static logger = new Logger();

    public static Exception LogIfFailure( this Exception e ) {
        if( e != null )
            logger.Write( e.Message );
        return e;
    }
    public static Exception ShowDialogIfFailure( this Exception e ) {
        if( e != null )
            MessageBox.Show( e.Message );
        return e;
    }
    public static void ThrowIfFailure( this Exception e ) {
        if( e != null )
            Throw( e );
    }
}