Return to Revisions

First of all, I would question whether this code works correctly. Based on the names in your code, I assume that for example DoCompileCodeBase() should execute even when StopOnCompileFailure is false, but its result should be ignored. But that's not what happens: when StopOnCompileFailure is false, DoCompileCodeBase() doesn't even execute, because of short-circuiting evaluation of &&.

If what the code does is really what it should do (which is what I'm going to assume in the following text), then you should probably rename the settings properties.

Second, you can extract the checks and the Do method calls into a separate method (Simon's answer already suggested something similar):

// may need a better name
private bool ExecuteInternal()
{
    var errors = new List<string>();
    var logger = _logProvider.GetLogger(GetType().Name);
    var settings = Properties.Settings.Default;

    DoArchiveOldInstallers(errors, logger);

    if (settings.StopOnBackupCurrentInstallerFailure && !DoBackupCurrentVersion(errors, logger))
        return false;

    if (settings.StopOnUpdateCodeBaseFailure && !DoGetLatestVersion(errors, logger))
        return false;

    if (settings.StopOnCompileFailure && !DoCompileCodeBase(errors, logger))
        return false;

    if (settings.StopOnBuildInstallerFailure && !DoBuildInstaller(errors, logger))
        return false;

    return true;
}

public override void Execute()
{
    bool success = ExecuteInternal();
    OnExecutedCompleted(success);
}

Third, since all the Do methods are on the same object, you could change errors and logger into fields, so that you don't have to pass them around. Though I'm not completely convinced this is actually better, especially if Execute() is a method that's called over and over.

Fourth, you might consider associating all the settings properties with the right Do method using some kind of data structure:

private readonly Tuple<bool, Func<bool>>[] action;

// in constructor:
actions = new Tuple<bool, Func<bool>>[]
{
    new Tuple<bool, Func<bool>>(settings.StopOnBackupCurrentInstallerFailure, DoBackupCurrentVersion),
    new Tuple<bool, Func<bool>>(settings.StopOnUpdateCodeBaseFailure, DoGetLatestVersion),
    new Tuple<bool, Func<bool>>(settings.StopOnCompileFailure, DoCompileCodeBase),
    new Tuple<bool, Func<bool>>(settings.StopOnBuildInstallerFailure, DoBuildInstaller)
}

private bool ExecuteInternal()
{
    var errors = new List<string>();
    var logger = _logProvider.GetLogger(GetType().Name);
    var settings = Properties.Settings.Default;

    DoArchiveOldInstallers();

    foreach (var action in actions)
    {
        if (action.Item1 && !action.Item2())
            return false;
    }

    return true;
}

The syntax to initialize the list of Tuples is not great. You could improve that by having a special type TupleList<T1, T2> : List<Tuple<T1, T2>>, which would have a method like void Add(T1 item1, T2 item2). That way, the initialization simplifies to:

actions = new TupleList<bool, Func<bool>>
{
    { settings.StopOnBackupCurrentInstallerFailure, DoBackupCurrentVersion },
    { settings.StopOnUpdateCodeBaseFailure, DoGetLatestVersion },
    { settings.StopOnCompileFailure, DoCompileCodeBase },
    { settings.StopOnBuildInstallerFailure, DoBuildInstaller }
}

The next step after this could be encapsulating each action into its own object, so that you could do something like:

foreach (var action in actions)
{
    if (!action.Execute())
        return false;
}

And each action would be responsible for checking the right setting, if appropriate. (Actions that don't do any checks, like DoArchiveOldInstallers could be represented by a different type that inherits from the same base, or something like that.) But I'm not sure doing that would be worth it in this case.