Generating a URL

Question

I have a function that generates a URL using a stringbuilder:

    internal String GenerateUrl()
    {
        StringBuilder urlBuilder = new StringBuilder("http://");
        urlBuilder.Append(IpAddress);
        urlBuilder.Append("/set.cmd?" + "user=" + Username + "+");
        urlBuilder.Append("pass=" + Password + "+");

        switch (Command)
        {
            case CommandType.Undefined:
                throw new ArgumentException("Command type has not been set");

            case CommandType.SetPower:
                urlBuilder.Append("cmd=setpower+");

                for (int i = 0; i < (Ports.Length-1); i++)
                {
                    urlBuilder.Append(BooleanToStringPortRemap(Ports[i], i + 1) + "+");
                }

                urlBuilder.Append(BooleanToStringPortRemap(Ports[Ports.Length - 1], Ports.Length));
                break;

            case CommandType.ReadPower:
                urlBuilder.Append("cmd=getpower");
                break;

            case CommandType.ReadCurrent:
                urlBuilder.Append("cmd=getcurrent");
                break;
        }

        return urlBuilder.ToString();
    }

This scores quite badly for maintainability and cyclomatic complexity. My question is, is there a better way to construct a URL or if not is there a better way to construct one using a Stringbuilder than what I am currently doing?

Answer 1

This is a mess:

StringBuilder urlBuilder = new StringBuilder("http://");
urlBuilder.Append(IpAddress);
urlBuilder.Append("/set.cmd?" + "user=" + Username + "+");
urlBuilder.Append("pass=" + Password + "+");

Why concatenate when there's an AppendFormat method?

StringBuilder urlBuilder = new StringBuilder(1024);
urlBuilder.AppendFormat("http://{0}/set.cmd?user={1}+pass={2}+", IpAddress, Username, Password);

Considering the repeated use of the key=value pattern, I wonder: why not use a Dictionary<string, object>, fill that with all of the necessary values, and at the end use string.join to combine them all? Something like this:

var keyValues = new Dictionary<string, object>();
keyValues.Add("user", UserName);
keyValues.Add("pass", Password);
keyValues.Add("cmd", commandName);

return "?" + string.Join("+", keyValues.Select(x=>string.Format("{0}={1}", x.Key, x.Value)));

Answer 2

StringBuilder urlBuilder = new StringBuilder("http://");

Instead of using this constructor you should use the one taking the initial capacity as an argument. By default the StringBuilder is initialized with a capacity of 16 which is doubled each time the internal buffer is too small. Using a higher initial capacity will reduce the number of operations to increase the buffer size.

urlBuilder.Append(IpAddress);
urlBuilder.Append("/set.cmd?" + "user=" + Username + "+");
urlBuilder.Append("pass=" + Password + "+");

As you are using a StringBuilder you should use the provided fluent way to use its methods. The methods of the StringBuilder like e.g the Append() method returns the StringBuilder itself.

You should check whether the Command is CommandType.Undefined as the very first operation in that method; in this way no unneeded work will be done.

Instead of using Ports.Length-1 you should extract it to a variable. However, this could also be done in a better way if we knew how the BooleanToStringPortRemap() method looks.

internal String GenerateUrl()
{
    if (Command == CommandType.Undefined)
    {
        throw new ArgumentException("Command type has not been set");
    } 

    StringBuilder urlBuilder = new StringBuilder(1024);

    urlBuilder.Append("http://")
              .Append(IpAddress)
              .Append("/set.cmd?user=").Append(Username).Append("+")
              .Append("pass=").Append(Password).Append("+");

    switch (Command)
    {
        case CommandType.SetPower:

            urlBuilder.Append("cmd=setpower+");

            int portsCount = Ports.Length - 1;

            for (int i = 0; i < portsCount; i++)
            {
                urlBuilder.Append(BooleanToStringPortRemap(Ports[i], i + 1))
                          .Append("+");
            }

            urlBuilder.Append(BooleanToStringPortRemap(Ports[portsCount], Ports.Length));
            break;

        case CommandType.ReadPower:

            urlBuilder.Append("cmd=getpower");
            break;

        case CommandType.ReadCurrent:

            urlBuilder.Append("cmd=getcurrent");
            break;
    }

    return urlBuilder.ToString();
}

Answer 3

Your cyclomatic complexity is high because of the switch statement! We can get rid of it using two solutions. The first one would be inheritance of command builders. But that's overkill, you don't want to have 3 classes for 3 builders that simply returns a string. But maybe you'd accept having 3 methods for 3 builders. (As long as you don't have too many ways to build an URL, that'd be good!)

I used some C#6 features but it's nothing can be changed a little to work with other versions of C#!

First, as @BCdotWEB wrote, you should use Format, which I did using String interpolation in C#6. (You can use String.Format if you can't use C#6!)

I created a test class to show you how I'd do it.

public class Test
{
    private readonly Dictionary<CommandType, Func<String>> commandBuilders;

    //Properties that you already have : IpAddress,Username,Password,Ports

    public Test()
    {
        commandBuilders = new Dictionary<CommandType, Func<String>>();
        commandBuilders.Add(CommandType.SetPower, BuildSetPower);
        commandBuilders.Add(CommandType.ReadPower, BuildReadPower);
        commandBuilders.Add(CommandType.ReadCurrent, BuildReadCurrent);
    }

    private string GetUrlBaseFormat() => $"http://{IpAddress}/set.cmd?user={Username}+pass={Password}+";

    private string BuildSetPower()
    {
        var urlBuilder = new StringBuilder(GetUrlBaseFormat());
        urlBuilder.Append("cmd=setpower+");

        for (int i = 0; i < (Ports.Length-1); i++)
        {
            urlBuilder.Append(BooleanToStringPortRemap(Ports[i], i + 1) + "+");
        }

        urlBuilder.Append(BooleanToStringPortRemap(Ports[Ports.Length - 1], Ports.Length));

        return urlBuilder.ToString();
    }

    private string BuildReadPower() => $"{GetUrlBaseFormat()}cmd=getpower";
    private string BuildReadCurrent() => $"{GetUrlBaseFormat()}cmd=getcurrent";

    internal String GenerateUrl()
    {
        Func<string> urlBuilder;
        bool builderExists = commandBuilders.TryGetValue(Command, out urlBuilder);

        if(!builderExists)
        {
            throw new ArgumentException("Command type has not been set");
        }

        return urlBuilder();
    }
}

That's a simple example I made that could work out well in your code. We use a Dictionary to hold which Func<String> to call when given a certain Command.

Now, each way to build an URL has its own method, most of them are one-liners.

This way, you don't have the switch in your code, and if you happen to have more ways to build URL your switch wouldn't grow in complexity! (But you'd have more methods, solution incoming!)

You could argue that I'm just moving the problem. You don't have an high complexity now, but you have more methods in your class. You could move these methods to a static class that would help to build URLs. And if that ever becomes a problem, you could use inheritance to have multiple instance of.. IUrlBuilder. These are the solutions to deal with your high cyclomatic complexity.

Finally, as said @Heslacher, if we knew what is inside the BooleanToStringPortRemap method, we could rework it to use something like String.Join instead of using a for loop to join your ports.