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 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.
Solution
There are already two good answers here, but in addition:
- Consider using the C# type keywords (
boolintead ofBoolean,intinstead ofInt32,stringinstead ofString) - Consider using
Containsinstead ofIndexOf - 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
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);
}
}
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.
Instead of the if/elseif/else use a return statement:
return !String.IsNullOrEmpty(result) && result.IndexOf("p61=1") != -1;
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 ...
}