HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Switching on hardware ports

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
portsswitchinghardware

Problem

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 t

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#110538, answer score: 11

Revisions (0)

No revisions yet.