Converting IP Addresses to minimal binary formats

Question

Once more, I come to you for assistance! I have written a significant bit of code that converts IP Addresses from strings to minimal byte-arrays. (So that you can store them in a binary format instead of as a wasteful string.)

It currently works with IPv4 and IPv6 addresses, saving both to a 16-element byte-array. (The IPv4 addresses are padded with 0's at the beginning as per this RFC.

It currently is only capable of converting an IPv4 or IPv6 address to a byte-array, though I am going to be adding the reverse conversion soon as well.

I've omitted XML documentation from it to shorten it up to the relevant code, but you can view the code (as of this version) in it's entirety on GitHub.

As usual I appreciate all comments on anything you see here.

public class IpHelpers
{
    public static string IpToHex(string ip, bool padIp)
    {
        if (ip.Contains('.'))
        {
            return Ip4ToHex(ip, padIp);
        }
        else
        {
            return Ip6ToHex(ip, padIp);
        }
    }

    public static string Ip4ToHex(string ip, bool padIp) => Ip6ToHex(Ip4ToIp6(ip), padIp);

    public static string Ip4ToIp6(string ip)
    {
        string[] ipv4Strings = ip.Split('.');

        if (ipv4Strings.Length == 4)
        {
            string[] ipv4Bytes = StringArrayToHexArray(ipv4Strings);
            return "::" + ipv4Bytes[0] + ipv4Bytes[1] + ":" + ipv4Bytes[2] + ipv4Bytes[3];
        }

        throw new ArgumentException($"The provided IP of {ip} is not a valid IP address.");
    }

    public static string[] StringArrayToHexArray(string[] input) => input.ToList().Select(x => Convert.ToByte(x, 10).ToString("x")).ToArray();

    public static string Ip6ToHex(string ip, bool padIp)
    {
        string result = "0x";

        ip = ExpandIp6(ip);

        string[] ipv6Words = ip.Split(':');

        for (int i = 0; i < ipv6Words.Length; i++)
        {
            result += ipv6Words[i].PadLeft(4, '0');
        }

        if (!padIp)
        {
            result = StripLeadingZeroes(result);
        }

        return result;
    }

    public static string ExpandIp6(string ip)
    {
        string result = "";

        if (!ip.Contains("::"))
        {
            return ip;
        }

        int wordsRead = 0;

        string[] ipv6Sections = ip.Split(new string[] { "::" }, StringSplitOptions.None);

        string[] ipv6Section0Words = ipv6Sections[0].Split(':');
        string[] ipv6Section1Words = ipv6Sections[1].Split(':');

        if (ipv6Sections[0].Length > 0)
        {
            wordsRead += ipv6Section0Words.Length;
        }

        if (ipv6Sections[1].Length > 0)
        {
            wordsRead += ipv6Section1Words.Length;
        }

        for (int i = wordsRead; i <= 7; i++)
        {
            if (result != "")
            {
                result += ':';
            }
            result += '0';
        }

        if (ipv6Sections[1] != "")
        {
            result += ':' + ipv6Sections[1];
        }

        return result;
    }

    public static string StripLeadingZeroes(string input)
    {
        string result = "0x";

        if (input.StartsWith("0x", StringComparison.OrdinalIgnoreCase))
        {
            input = input.Substring(2);
        }

        bool inIp = false;

        for (int i = 0; i < input.Length; i+=2)
        {
            if (input[i] != '0' || input[i+1] != '0')
            {
                inIp = true;
            }

            if (inIp)
            {
                result += input[i];
                result += input[i + 1];
            }
        }

        return result;
    }

    public static byte[] HexToByteArray(string hex)
    {
        if (hex[0] == '0' && (hex[1] == 'x' || hex[1] == 'X'))
        {
            hex = hex.Substring(2);
        }

        byte[] result = new byte[hex.Length / 2];

        for (int i = 0; i < result.Length; i++)
        {
            result[i] = Convert.ToByte(hex.Substring(i * 2, 2), 16);
        }

        return result;
    }
}

I only have a few relevant Unit Tests, so feel free to comment on them as well.

[TestClass]
public class IpHelpersTests
{
    [TestMethod]
    public void IPv4ToHex_192_168_0_1()
    {
        var expected = "0x000000000000000000000000c0a80001";
        var input = "192.168.0.1";

        var actual = Framework.Utilities.IpHelpers.IpToHex(input, true);

        Assert.AreEqual(expected, actual);
    }

    [TestMethod]
    public void HexToByteArray_0x000000000000000000000000c0a80001()
    {
        var expected = new byte[] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xC0, 0xA8, 0x00, 0x01 };
        var input = "0x000000000000000000000000c0a80001";

        var actual = Framework.Utilities.IpHelpers.HexToByteArray(input);

        CollectionAssert.AreEqual(expected, actual);
    }

    [TestMethod]
    public void IPv6ToHex_1()
    {
        var expected = "0x00000000000000000000000000000001";
        var input = "0000:0000:0000:0000:0000:0000:0000:0001";

        var actual = Framework.Utilities.IpHelpers.IpToHex(input, true);

        Assert.AreEqual(expected, actual);
    }

    [TestMethod]
    public void ExpandIPv6_CC1()
    {
        var expected = "0:0:0:0:0:0:0:1";
        var input = "::1";

        var actual = Framework.Utilities.IpHelpers.ExpandIp6(input);

        Assert.AreEqual(expected, actual);
    }

    [TestMethod]
    public void StripLeadingZeroes_0x00000000000000000000000000000001()
    {
        var expected = "0x01";
        var input = "0x00000000000000000000000000000001";

        var actual = Framework.Utilities.IpHelpers.StripLeadingZeroes(input);

        Assert.AreEqual(expected, actual);
    }

    [TestMethod]
    public void StripLeadingZeroes_0x000000000000000000000000c0a80001()
    {
        var expected = "0xc0a80001";
        var input = "0x000000000000000000000000c0a80001";

        var actual = Framework.Utilities.IpHelpers.StripLeadingZeroes(input);

        Assert.AreEqual(expected, actual);
    }
}

Answer 1

public class IpHelpers

1. Your class has only static methods but it's not marked as static, you should change it to public static class IpHelpers. It'll prevent to accidently forget to add static in some methods.

Your first public function:

public static string IpToHex(string ip, bool padIp)

In my opinion it has these issues:

2. You accept IP address as string however you do not perform any validation. A completely invalid address will result in garbage output or unclear run-time errors.

I assume it's a public function (both function and class are public) then you should always validate your inputs. I'd rely on existing .NET classes to do not reinvent the wheel:

public static string IpToHex(IPAddress ip, bool padId)

3. Now you have a boolean parameter that change function behavior. It's bad because at calling point you can't quickly understand what true means and if you need to add more options then you will need more and more parameters.

Instead you should use an enum:

[Flags]
public enum IpToHexOptions {
    None = 0,
    Pad = 1
}

Function prototype will then be:

public static string IpToHex(IPAddress ip, IpToHexOptions options)

You may want to make second parameter optional with IpToHexOptions options = IpToHexOptions.None. Do not forget to also validate enum value.

5. Using one centralized entry point will also let you drop all your IpXyzToHex functions. If you want to provide an overload version with a string input then simply rely on IPAddress.Parse() function.

6. Functions StripLeadingZeroes and Hex2ByteArray are public but they're just implementation details (otherwise they shouldn't stay in your IpHelpers class but in another place). Make them private. If you made them public because of test methods then you should refactor them out or make tests assembly a friend or (better, IMO) test them trough your class public interface IpToHex method.

7. In your ExpandIp6() function you build output string on result variable. It's terribly slow because you're allocating new strings multiple times. Use a StringBuilder instead (you can even set its initial capacity to desired value because it's known).

8. To strip leading zeros you may use TrimLeft('0') method (skipping first two characters).

I. All these said you may take a look to IPAddress.GetAddressBytes() method, it will greatly simplify your code.

II. Last step is...class itself. IpHelpers smells bad. An helper class is often (not always!) a symptom you have something you don't know where to put. It may be an extension method like this:

public static IPAddressExtensions {
    public static string ToHex(this IPAddress address, IPAddressToHexOptions options) {
        // Your implementation
    }
}

III. You may even accept a string parameter to specify formatting options (like you're used to do with IFormattable.ToString()) such as lower-case or upper-case hexadecimal numbers (or padding):

public static string ToHex(this IPAddress address, string format)

Try to be as much coherent with Framework conventions and it'll integrate without trouble.