Switching on hardware ports

Question

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.

Answer 1

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

Answer 2

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

Answer 3

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.

Answer 4

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

return !String.IsNullOrEmpty(result) && result.IndexOf("p61=1") != -1;

Answer 5

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 ...
}

Answer 6

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