3 gosh, the mea-culpa fix was wrong!
source | link

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.

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.

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.

2 added mea culpa
source | link

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.

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

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.

    Tweeted twitter.com/#!/StackCodeReview/status/403644507016949760
1
source | link

Multiple return statements smells like what?

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