10
\$\begingroup\$

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.

\$\endgroup\$
12
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ BTW, I also considered using calling OnExecutedCompleted() once in a finally, but I think that would be an abuse of the feature. \$\endgroup\$ – svick Nov 21 '13 at 18:02
  • 1
    \$\begingroup\$ I think I'd rather have a Dictionary<bool, Func<bool>> than a Tuple<bool, Func<bool>>[]. Wow you guys don't see it, but the rest of this code is so ugly, it's unreal! Can't believe I wrote that crap less than 6 months ago! \$\endgroup\$ – Mathieu Guindon Nov 21 '13 at 18:40
  • 1
    \$\begingroup\$ @retailcoder Nope, Dictionary wouldn't work here, because it could have only one Func for true and one for false. \$\endgroup\$ – svick Nov 21 '13 at 18:47
  • 2
    \$\begingroup\$ @retailcoder Yeah, I considered something like that. But I think that in this case, the foreach is clearer. LINQ works best for queries, i.e. when there are no side effects. \$\endgroup\$ – svick Nov 21 '13 at 19:28
  • 1
    \$\begingroup\$ As for what type of smell? The code smells of REPETITION and RIGIDITY. Both covered nicely by the rewrites suggested. \$\endgroup\$ – James Snell Nov 21 '13 at 20:56
6
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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)

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think executeIsCompleted is a good name for that method, because it doesn't just test whether execution is complete, it also performs the execution itself. \$\endgroup\$ – svick Nov 21 '13 at 17:33
  • 1
    \$\begingroup\$ Smells like Java code :) (sees edit and LOL!) \$\endgroup\$ – Mathieu Guindon Nov 21 '13 at 17:34
  • \$\begingroup\$ @svick I agree, as I currently can't come up with a better name, I renamed it to todoRenameMeToWhatYouThinkSuitsYou (I didn't put much thought into the naming of the method as that wasn't the most important part of the question) \$\endgroup\$ – Simon Forsberg Nov 21 '13 at 17:35
  • 1
    \$\begingroup\$ @retailcoder Java code doesn't smell (at least not my code :P). I hope this solves the multiple return statements for you though. \$\endgroup\$ – Simon Forsberg Nov 21 '13 at 17:38
  • 3
    \$\begingroup\$ Also, obj is a very bad name for a variable. I would use something like settings. \$\endgroup\$ – svick Nov 21 '13 at 17:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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