4
\$\begingroup\$

I have the following function for turning on some ports on a piece of hardware:

public Boolean SwitchOn(Int32 portNumber)
    {
        HttpCommandBuilder builder = new HttpCommandBuilder(_ipAddress);
        builder.Command = HttpCommandBuilder.CommandType.SetPower;

        String result;

        if (portNumber == 1)
        {
            builder.Port1 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p61=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else if (portNumber == 2)
        {
            builder.Port2 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p62=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else if (portNumber == 3)
        {
            builder.Port3 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p63=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else if (portNumber == 4)
        {
            builder.Port4 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p64=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else
        {
            throw new ArgumentException("Invalid port number: " + portNumber);
        }
    }

It has a low maintainability index and there's a lot of copy and pasting going on and I am doing a similar, repetitive action for each port.

I know it's not a very elegant solution so I am looking for any advice and suggestions on how I could re-factor this function to improve its maintainability score.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. \$\endgroup\$ – SuperBiasedMan Nov 12 '15 at 12:19
  • \$\begingroup\$ Is it possible that SendCommand() returns null ? \$\endgroup\$ – Heslacher Nov 12 '15 at 12:30
  • 3
    \$\begingroup\$ Are you using Visual Studio (and not Mono)? Consider using a tool like Resharper. I'm always amazed at the hints it gives me - code consistency as well as refactoring help. Well worth the price of admission, even for a part time programmer like me. \$\endgroup\$ – WernerCD Nov 12 '15 at 14:00
  • \$\begingroup\$ @WernerCD It is also free for students, as long as you have a .edu or .edu.something email, or can send some sort of proof to them. \$\endgroup\$ – Kroltan Nov 12 '15 at 23:00
11
\$\begingroup\$

There are already two good answers here, but in addition:

  • Consider using the C# type keywords (bool intead of Boolean, int instead of Int32, string instead of String)
  • Consider using Contains instead of IndexOf
  • Consider introducing a method to set the port. If you can't modify HttpCommandBuilder, an extension method is one option.
  • Consider using object initializers

Here's an example with these suggestions put together:

public bool SwitchOn2(int portNumber)
{
    var builder = new HttpCommandBuilder(_ipAddress)
    {
        Command = HttpCommandBuilder.CommandType.SetPower
    };

    builder.SetPort(portNumber);
    var result = SendCommand(builder.GenerateUrl());
    return !string.IsNullOrEmpty(result) && result.Contains($"p6{portNumber}=1");
}

internal static class HttpCommandBuilderExtensions
{
    public static void SetPort(this HttpCommandBuilder builder, int portNumber)
    {
        if (builder == null)
        {
            throw new ArgumentNullException(nameof(builder));
        }

        switch (portNumber)
        {
            case 1:
                builder.Port1 = true;
                break;
            case 2:
                builder.Port2 = true;
                break;
            case 3:
                builder.Port3 = true;
                break;
            case 4:
                builder.Port4 = true;
                break;
            default:
                throw new ArgumentOutOfRangeException(nameof(portNumber));
        }
    }
}

For what it's worth, here are the code metrics from VS2015:

Member: SwitchOn2(int) : bool
Maintainability Index: 70
Cyclomatic Complexity: 2
Class Coupling: 2
Lines of Code: 5

Member: SetPort(this HttpCommandBuilder, int) : void
Maintainability Index: 62
Cyclomatic Complexity: 6
Class Coupling: 4
Lines of Code: 12

And for the original:

Member: SwitchOn(int) : bool
Maintainability Index: 45
Cyclomatic Complexity: 13
Class Coupling: 2
Lines of Code: 36
\$\endgroup\$
7
\$\begingroup\$

Use a switch(portNumber) instead of if... else if... else...:

public Boolean SwitchOn(Int32 portNumber)
{
    HttpCommandBuilder builder = new HttpCommandBuilder(_ipAddress);
    builder.Command = HttpCommandBuilder.CommandType.SetPower;

    String result;

    switch(portNumber)
    {
        case 1:
            // other code
            break;
        case 2:
            // other code
            break;
        case 3:
            // other code
            break;
        case 4:
            // other code
            break;
        default:
            throw new ArgumentException("Invalid port number: " + portNumber);
    }
}
\$\endgroup\$
6
\$\begingroup\$

This

        result = SendCommand(builder.GenerateUrl());

        if (String.IsNullOrEmpty(result))
        {
            return false;
        }
        else if (result.IndexOf("p64=1") == -1)
        {
            return false;
        }
        else
        {
            return true;
        }  

is (almost) duplicated code for each branch of the if..else if... The only difference is the string that is searched using IndexOf() but this can be constructed with the help of the portNumber.

Let us extract the validation of the result to a separate method to get it out of the way.

private bool ValidateResponse(string response, int portNumber)
{
    if (response == null) { return false; } // assuming izt can be null, otherwise this can be removed.

    string query = string.Format("p6{0}=1", portNumber);

    return response.IndexOf(query) != -1;
}  

which results in the former SwitchOn() method looking like so

public bool SwitchOn(int portNumber)
{

    HttpCommandBuilder builder = new HttpCommandBuilder(_ipAddress);
    builder.Command = HttpCommandBuilder.CommandType.SetPower;

    switch (portNumber)
    {
        case 1:
            builder.Port1 = true;
            break;

        case 2:
            builder.Port2 = true;
            break;

        case 3:
            builder.Port3 = true;
            break;

        case 4:
            builder.Port4 = true;
            break;

        default:
            throw new ArgumentException("Invalid port number: " + portNumber);
    }

    string response = SendCommand(builder.GenerateUrl());

    return ValidateResponse(response, portNumber);
}  

As you see this you will have noticed that I have renamed result to response because it seems a better fit.

I have also used the alias bool and int because nowadays C# developers are used to use them.

\$\endgroup\$
5
\$\begingroup\$

Instead of the if/elseif/else use a return statement:

return !String.IsNullOrEmpty(result) && result.IndexOf("p61=1") != -1;
\$\endgroup\$
4
\$\begingroup\$

At a quick glance I did not see anyone else mention this.

You are giving each port its own variable.

However, look at their names - Port1, Port2, Port3, Port4 ... PortN.

The ports are not independent variables. They form a sequence. Dont hide this fact from the compiler. The fact you are hiding information and then manually 'hardcoding' structure, is why there is repetition in your code.

Store the ports using an array instead. Then repetition is no longer necessary since the same code works for ALL the ports, just use the correct number to index into the port array. Store the html (?) snippets in an array of strings as well (which also brings them to a single place, currently they are hardcoded almost like 'magic numbers').

I do not know C#, so here is the general structure im suggesting:

function setPort(i)
{
    array of html snippets = {"p61", "p62", "p63", "p64"}; //can be supplied from outside if appropriate
    *throw if number of snippets is less than i*
    *initialize builder*
    builder.Ports[i] = true; //possibly use a method like suggested
    *send the thing*
    *check the result*
    if (result is empty) ...
    else if (result.IndexOf(snippets[i]) == -1) ...
    else ...
}
\$\endgroup\$
0
\$\begingroup\$

Thanks for all the feedback, I have taken suggestions from all the answers. I cannot use type keywords because my organisation's coding style guide dictates that we use the full class name.

Here is the re-factored function:

public Boolean SwitchOn(Int32 portNumber)
    {
        HttpCommandBuilder builder = new HttpCommandBuilder(_ipAddress);
        builder.Command = HttpCommandBuilder.CommandType.SetPower;
        builder.SetPort(portNumber, true);

        var result = SendCommand(builder.GenerateUrl());
        return !String.IsNullOrEmpty(result) && result.Contains($"p6{portNumber}=1");
    }

And the new function in HttpCommandBuilder:

public void SetPort(Int32 portNumber, Boolean state)
    {
        if (state)
        {
            switch (portNumber)
            {
                case 1:
                    Port1 = true;
                    break;
                case 2:
                    Port2 = true;
                    break;
                case 3:
                    Port3 = true;
                    break;
                case 4:
                    Port4 = true;
                    break;
                default:
                    throw new ArgumentOutOfRangeException("Cannot set port number " + portNumber);
            }
        }
        else
        {
            switch (portNumber)
            {
                case 1:
                    Port1 = false;
                    break;
                case 2:
                    Port2 = false;
                    break;
                case 3:
                    Port3 = false;
                    break;
                case 4:
                    Port4 = false;
                    break;
                default:
                    throw new ArgumentOutOfRangeException("Cannot set port number " + portNumber);
            }
        }
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ I think you can drop the if-else condition and transform the PortX = true/false statements to just PortX = state; \$\endgroup\$ – Gentian Kasa Nov 12 '15 at 16:56

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.