5
\$\begingroup\$

The point of this code is to build an ip from a list of type ushort.

var ip = new StringBuilder();
List<ushort> ipList = new List<ushort>(4) {192, 168, 1, 1};

ipList.ToList().ForEach(x => ip.Append(x + "."));

return ip.Remove(ip.Length - 1, 1).ToString();

The code works and outputs an ip as expected, but the formatting I give it leaves to be desired, having to delete the last element of the string does not look like a reliable solution or at least, I don't feel like it is.

The code above would output, before returning, the following string:

"192.168.1.1."

And after removing the last character it will look like this:

"192.168.1.1"

\$\endgroup\$
  • \$\begingroup\$ You do know about the IpAddress class, right? \$\endgroup\$ – BCdotWEB Nov 27 '15 at 11:14
  • \$\begingroup\$ No I didn't, but I think the use I am going to give to the string does not need this class since I just have to check an IP has been given to my device through DHCP, I don't have to interact with the ip in any way \$\endgroup\$ – Oscar Guillamon Nov 27 '15 at 11:29
12
\$\begingroup\$

There is a string.Join() method which would exactly do what you want like so

string ip = string.Join(".", ipList);

btw, you don't need to call ToList() on a List<T>.

\$\endgroup\$
  • \$\begingroup\$ I like this one because it is straightforward. Also, the tolist is a typo from my original code, I tried to provide an example without any of my code exported here, the data is given to me in an array, that's why the ToList exists \$\endgroup\$ – Oscar Guillamon Nov 27 '15 at 10:55
  • \$\begingroup\$ Ok, I will correct my code then, thanks for pointing that out, \$\endgroup\$ – Oscar Guillamon Nov 27 '15 at 10:58
10
\$\begingroup\$

The biggest problem here is that an IPv4 address is really four unsigned 8 bit integers, but you're using 16 bit unsigned integers to represent them.

You should be using an array of bytes instead of ushorts.

Obviously, you'll still want to leverage the other answers and Join them as well.

\$\endgroup\$
  • \$\begingroup\$ I am using 16 bit unsigned integers because it's the format used by the device to return data since it uses the modbus protocol. \$\endgroup\$ – Oscar Guillamon Nov 27 '15 at 11:58
  • 3
    \$\begingroup\$ Why does the input dictate your representation of the data? Don't let some other programmer's poor decision "force" you into one. Create an adapter that transforms them into a proper representation. Hell, I'd consider creating your own IPv4 struct and put that call to Join in a ToString method. \$\endgroup\$ – RubberDuck Nov 27 '15 at 12:15
  • \$\begingroup\$ I can't argue you about that, i will create one for this and future matters regarding this. I make sometimes questions which can look stupid, but my work does not revolve around optimal code rather than "resilient" code and I don't want that to affect my coding practices, I've seen such things....... \$\endgroup\$ – Oscar Guillamon Nov 27 '15 at 12:18
  • 1
    \$\begingroup\$ For giggles, I implemented an IPv4 struct. codereview.stackexchange.com/questions/112088/… \$\endgroup\$ – RubberDuck Nov 27 '15 at 21:46
  • \$\begingroup\$ That is fantastic, thanks for the time you invested in making that struct! \$\endgroup\$ – Oscar Guillamon Nov 30 '15 at 9:03
4
\$\begingroup\$

For those who have to use an old .NET Framework (prior to 4.0).
To eliminate removing of the last character, you could use the String.Join method:

return String.Join(".", ipList.ConvertAll(x => x.ToString("0")).ToArray());
\$\endgroup\$
4
\$\begingroup\$

I had not read all answers/comments before beginning this, so the answer might be a bit out of scope if all of those were taken into account. However, I still think this extension method might be useful for someone else, so I'll leave it here.

    public static IPAddress ToIPv4Address(this IReadOnlyCollection<ushort> source)
    {
        if (source == null)
        {
            throw new ArgumentNullException(nameof(source));
        }

        if (source.Count != 4)
        {
            throw new ArgumentException("An IPv4 address must consist of four bytes", nameof(source));
        }

        var invalidBytes = source.Where(v => v > byte.MaxValue).ToList();
        if (invalidBytes.Count > 0)
        {
            throw new ArgumentException($"{nameof(source)} contains items that are out of the valid range for a byte: {string.Join(", ", invalidBytes)}", nameof(source));
        }

        return new IPAddress(source.Select(s => (byte)s).ToArray());
    }

This allows us to just use the built in ToString capabilities of the IPAddressclass.

Console.WriteLine(new List<ushort>(4) { 192, 168, 1, 1 }.ToIPv4Address());

Additionally, this obviously checks that all items of the List<ushort> are in a valid range for an IPv4 address and that the source list has exactly four items.

\$\endgroup\$
  • \$\begingroup\$ @Heslacher Thanks. I saved the invalid items into a list, so that I could add them to the exception message, but maybe re-evaluating the expression would not really matter in that case anyways, since the exception bubbling up is probably the slowest part anyways. Not really decided about that one. :) \$\endgroup\$ – hangy Nov 27 '15 at 13:05

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.