Multiple return statements smells like what?

Question

Here's some code of mine that I don't like (this was my very first attempt at DI/IoC!). It's a small C# application that runs scheduled on one of our servers and is responsible for fetching the latest version of a VB6 codebase, compiling it and packaging an installer for it.

There's an Execute method in there that smells like... well you tell me what it smells like:

    /// <summary>
    /// Executes the build process.
    /// </summary>
    public override void Execute()
    {
        var errors = new List<string>();
        var logger = _logProvider.GetLogger(GetType().Name);

        DoArchiveOldInstallers(errors, logger);
        if (Properties.Settings.Default.StopOnBackupCurrentInstallerFailure && !DoBackupCurrentVersion(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        if (Properties.Settings.Default.StopOnUpdateCodeBaseFailure && !DoGetLatestVersion(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        if (Properties.Settings.Default.StopOnCompileFailure && !DoCompileCodeBase(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        if (Properties.Settings.Default.StopOnBuildInstallerFailure && !DoBuildInstaller(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        OnExecutedCompleted(true);
    }

    private void OnExecutedCompleted(bool success)
    {
        if (Completed != null) Completed(this, new CompletedEventArgs(success));
    }

---

Mea Culpa

Sorry this was posted like this. The conditions should read as follows:

if (!DoXXXXX(errors, logger) && !settings.StopOnXXXXXFailure)

This was a last-minute untested change I made just minutes prior to posting, without realizing it would totally mess up the method's logic.

Answer 1

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.

Answer 2

First thing I noticed, I do not like the Do in the function names. For instance, in DoArchiveOldInstallers there are two verbs. The idea is to mimic english, I never say "Do go for a run" when I'm talking to people. The code should be the same. ArchiveOldInstallers works perfectly well, and the verb describing what is going on is still there.

Second, the if statements get a little confusing. I'm not sure what to do with them right now, but they need to be changed.

Other than that, the white space, capitalization, and formatting look good.

Answer 3

I would create a helper method for returning the value you want to give to OnExecutedCompleted, and also store the value of Properties.Settings.Default in a variable.

bool TodoRenameMeToWhatYouThinkSuitsYou()
{
    SomeType settings = Properties.Settings.Default;
    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;
}

Then you can replace the smelly part in your original method with

OnExecutedCompleted(TodoRenameMeToWhatYouThinkSuitsYou());

(I apoligize for not following the coding conventions in this answer, I am not a C# developer)