CODEWITHSUNDEEP
c#delegates
KeesDijk
KeesDijk

Reputation: 2329

Multiple methods that need to be handled the same way

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:

Thanks.

Upvotes: 1

Views: 245

Answers (4)

jyoung
jyoung

Reputation: 5161

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 );
    }
}

Upvotes: 0

Marc Gravell
Marc Gravell

Reputation: 1062530

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);

Upvotes: 0

David Arno
David Arno

Reputation: 43254

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.

Upvotes: 2

Jon Skeet
Jon Skeet

Reputation: 1499750

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.

Upvotes: 9

Related Questions