IPv4 struct: Round 2

Question

Summary

Original Question here. Updated GitHub link here

I've taken an interest in lower level data structures and I implemented an 'Ip4Address` to get familiar with Explicit structure layout. It turned out to be an exercise in properly implementing a struct in general, but I digress. The idea here is that each Octet of the IPv4 address is represented by a byte in a single 32 bit unsigned integer.

Things I've addressed since last review:

• Better argument validation all around.
• The struct is now immutable.
• Extracted a method for validating Ip addresses passed as strings.
• The code no longer needlessly initializes the struct only to overwrite it's values.
• Implemented IEquatable & IComparable.
  • In addition, I implemented and overrode the other methods that are recommended when implementing those two interfaces. This includes Object.Equals(Object), GetHashCode(), and the equality operators.
• Added missing test cases.
• Implemented a ToByteArray() method.
• Removed useless variables from test cases.
• Use a method group for address.Split('.').Select(a => Byte.Parse(a)).ToArray()
  • I'm still on the fence about this last one. I like the explicitness of the syntax I originally used, but I changed it anyway.

Things I did not address:

• Using Assert.Throws() for validating exceptions in my tests.
  • I'm not switching to NUnit. I like MSTest.
• Expression bodied member for ToString() override.
  • I'm hanging onto my curly braces for now. Expression bodied members are still feeling really weird to me.
• Properties for the Octets.
  • It felt really weird to explicitly lay out the structure of the struct, then to hide it all behind properties. I'm open to hearing a solid argument for doing this though. I just wasn't convinced during my last review.
• Use uint instead of UInt32
  • I explicitly used UInt32 to make it clear that Address is a 32 bit integer and that the Octets use the same memory space.

Questions:

• Did I get the GetHashCode() implementation right? I'm never really sure I that I got it right.
• My CompareTo() implementation feels messy. It's better than my original implementation of it, but still doesn't feel very good.
• Does this code suffer from any Big Endian vs Little Endian issues? It was mentioned in my last review, but I wasn't able to confirm/refute it.
• As always, I'm open to any & all feedback.

Ip4Address.cs

using System;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text.RegularExpressions;

namespace Rubberduck.Katas.Network
{
    [StructLayout(LayoutKind.Explicit)]
    public struct Ip4Address : IEquatable<Ip4Address>, IComparable<Ip4Address>
    {
        /// <summary>
        /// Represents the Base Ten IPv4 address as a raw integer.
        /// </summary>
        /// <remarks>Overlays the Octet fields, so changing this value changes the Octets & vice versa.</remarks>
        [FieldOffset(0)]
        // ReSharper disable once BuiltInTypeReferenceStyle
        public readonly UInt32 Address;

        // Each Octet is mapped to a byte of the address.
        [FieldOffset(0)]
        public readonly byte Octet1;
        [FieldOffset(1)]
        public readonly byte Octet2;
        [FieldOffset(2)]
        public readonly byte Octet3;
        [FieldOffset(3)]
        public readonly byte Octet4;

        /// <summary>
        /// Creates a new Ip4Address from a byte array.
        /// </summary>
        /// <param name="address">
        /// Must be an array of Length 4. 
        /// Index 0 is mapped to the first octet.
        /// </param>
        public Ip4Address(byte[] address)
        {
            if (address == null)
            {
                throw new ArgumentNullException(nameof(address));
            }

            const int expectedLength = 4;

            if (address.Length != expectedLength)
            {
                throw new ArgumentException($"{nameof(address)} array must have a length of {expectedLength}.", nameof(address));
            }

            // Set address because we must set all fields in the struct, else there is a compiler error.
            // It seems the compiler isn't aware that they're really the same thing.
            // We could call `:this()`, but I don't want to initalize it before arg checking.
            Address = 0;

            Octet1 = address[0];
            Octet2 = address[1];
            Octet3 = address[2];
            Octet4 = address[3];
        }

        /// <summary>
        /// Creates a new Ip4Address from it's base ten representation.
        /// </summary>
        /// <param name="address">Base ten representation of an IPv4 address. 
        /// UInt32.MaxValue results in an IP of "255.255.255.255".
        /// </param>
        public Ip4Address(UInt32 address)
            : this()
        {
            Address = address;
        }

        /// <summary>
        /// Creates a new Ip4Address from a well formed IP address. i.e. "10.10.1.255"
        /// </summary>
        /// <param name="address"></param>
        /// <exception cref="ArgumentNullException"></exception>
        /// <exception cref="ArgumentException">If the <paramref name="address"/> is not a valid IPv4 address.</exception>
        public Ip4Address(string address)
            : this(ParseStringAddress(address))
        { }

        // Using a private method because this work must be done prior to passing it off to a chained ctor call.
        private static byte[] ParseStringAddress(string address)
        {
            if (address == null)
            {
                throw new ArgumentNullException(nameof(address));
            }

            // Validation pattern shamelessly borrowed from http://www.regextester.com/22
            // It validates not only the format, but the number ranges too, 
            // so by time we're casting to a byte, it's a safe operation.
            var ipRegex = new Regex(@"^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$");

            if (!ipRegex.IsMatch(address))
            {
                throw new ArgumentException($"{address} is not a valid IPv4 address.", nameof(address));
            }

            return address.Split('.').Select(Byte.Parse).ToArray();
        }

        /// <summary>
        /// Indicates whether this instance and a specified object are equal.
        /// </summary>
        /// <returns>
        /// true if <paramref name="obj"/> and this instance are the same type and represent the same value; otherwise, false. 
        /// </returns>
        /// <param name="obj">The object to compare with the current instance. </param>
        public override bool Equals(object obj)
        {
            if (!(obj is Ip4Address))
            {
                return false;
            }

            return this.Equals((Ip4Address)obj);
        }

        /// <summary>
        /// Indicates whether the current object is equal to another object of the same type.
        /// </summary>
        /// <returns>
        /// true if the current object is equal to the <paramref name="other"/> parameter; otherwise, false.
        /// </returns>
        /// <param name="other">An object to compare with this object.</param>
        public bool Equals(Ip4Address other)
        {
            return Address.Equals(other.Address);
        }

        public static bool operator ==(Ip4Address ip1, Ip4Address ip2)
        {
            return ip1.Equals(ip2);
        }

        public static bool operator !=(Ip4Address ip1, Ip4Address ip2)
        {
            return !(ip1 == ip2);
        }

        /// <summary>
        /// Compares the current object with another object of the same type by doing a byte by byte comparison of the Octets.
        /// </summary>
        /// <returns>
        /// A value that indicates the relative order of the objects being compared. The return value has the following meanings: Value Meaning Less than zero This object is less than the <paramref name="other"/> parameter.Zero This object is equal to <paramref name="other"/>. Greater than zero This object is greater than <paramref name="other"/>. 
        /// </returns>
        /// <param name="other">An Ip4Address to compare with this object.</param>
        /// <remarks>
        /// Comparing the <seealso cref="Address"/> is not sufficient because it represents 4 bytes (Octets) of data, not a single integer.
        /// Therefore, this implementation does an Octet by Octet comparison.
        /// </remarks>
        public int CompareTo(Ip4Address other)
        {
            if (this.Equals(other))
            {
                return 0;
            }

            var octet1Comparison = Octet1.CompareTo(other.Octet1);

            if (octet1Comparison != 0)
            {
                return octet1Comparison;
            }

            var octet2Comparison = Octet2.CompareTo(other.Octet2);

            if (octet2Comparison != 0)
            {
                return octet2Comparison;
            }

            var octet3Comparison = Octet3.CompareTo(other.Octet3);

            if (octet3Comparison != 0)
            {
                return octet3Comparison;
            }

            // Should never return 0 at this point, because we checked very early if they were equal.
            return Octet4.CompareTo(other.Octet4);
        }

        /// <summary>
        /// Returns the hash code for this instance.
        /// </summary>
        /// <returns>
        /// A 32-bit signed integer that is the hash code for this instance.
        /// </returns>
        public override int GetHashCode()
        {
            return Address.GetHashCode();
        }

        public override string ToString()
        {
            return $"{Octet1}.{Octet2}.{Octet3}.{Octet4}";
        }

        /// <summary>
        /// Returns the Octets in the form of a byte array. 
        /// </summary>
        /// <returns>
        /// A byte array of length 4, containing the Octets of the address.
        /// <see cref="Octet1"/> is mapped to index 0 of the array.
        /// </returns>
        public byte[] ToByteArray()
        {
            return new byte[] { Octet1, Octet2, Octet3, Octet4 };
        }
    }
}

The Tests

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace Rubberduck.Katas.Network.Tests
{
    [TestClass]
    public class Ip4AddressTests
    {
        [TestMethod]
        public void CanCreateFromString()
        {
            Ip4Address ip = new Ip4Address("192.10.1.1");

            Assert.AreEqual(192, ip.Octet1);
            Assert.AreEqual(10, ip.Octet2);
            Assert.AreEqual(1, ip.Octet3);
            Assert.AreEqual(1, ip.Octet4);
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void WhenStringHasSpaces_ThrowsArgException()
        {
            new Ip4Address("1 . 2 . 3 . 4");
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void WhenStringIsMalformed_ThrowsArgException()
        {
            new Ip4Address("10.10.1..");
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void WhenStringHasAlphaChars_ThrowsArgException()
        {
            new Ip4Address("10.10.A.1");
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void WhenStringIsWellFormedButNotAValidIpAddress_ThrowsArgException()
        {
            new Ip4Address("123.456.789.999");
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentNullException))]
        public void WhenByteArrayArgIsNull_ThrowsNullArgException()
        {
            new Ip4Address((byte[])null);
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentNullException))]
        public void WhenStringArgIsNull_ThrowsNullArgException()
        {
            new Ip4Address((string)null);
        }

        [TestMethod]
        public void CanCreateFromByteArrayAndGetEquivalentArrayBack()
        {
            var expected = new byte[] { 192, 10, 1, 1 };

            var ip = new Ip4Address(expected);

            CollectionAssert.AreEqual(expected, ip.ToByteArray());
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void ByteArrayLengthCannotBeLessThan4()
        {
            new Ip4Address(new byte[] { 192, 10, 1 });
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void ByteArrayLengthCannotBeGreaterThan4()
        {
            new Ip4Address(new byte[] { 1, 1, 1, 1, 1 });
        }

        [TestMethod]
        public void CanCreateFromBaseTenAddress()
        {
            //i.e. can create from an integer.
            var ip = new Ip4Address(UInt32.MaxValue); //0xFFFFFFFF

            Assert.AreEqual(255, ip.Octet1);
            Assert.AreEqual(255, ip.Octet2);
            Assert.AreEqual(255, ip.Octet3);
            Assert.AreEqual(255, ip.Octet4);
        }

        [TestMethod]
        public void ToStringReturnsExpectedResult()
        {
            var ip = new Ip4Address(new byte[] { 10, 10, 1, 1 });

            Assert.AreEqual("10.10.1.1", ip.ToString());
        }

        [TestMethod]
        public void WhenTwoIpAddressesAreTheSame_TheyAreEqual()
        {
            var ip1 = new Ip4Address("10.10.1.1");
            var ip2 = new Ip4Address("10.10.1.1");

            //Assert.AreEqual calls Object.Equals(Object), so test both IEquatable & Object.Equals override.
            Assert.IsTrue(ip1.Equals(ip2));
            Assert.AreEqual(ip1, ip2);
            Assert.IsTrue(ip1 == ip2);
        }

        [TestMethod]
        public void WhenTwoIpAddressesAreNotTheSame_TheyAreNotEqual()
        {
            var ip1 = new Ip4Address("10.10.1.1");
            var ip2 = new Ip4Address("192.10.1.1");

            //Assert.AreEqual calls Object.Equals(Object), so test both IEquatable & Object.Equals override.
            Assert.IsFalse(ip1.Equals(ip2));
            Assert.AreNotEqual(ip1, ip2);
            Assert.IsTrue(ip1 != ip2);
        }

        [TestMethod]
        public void CompareTo_GreaterThan()
        {
            var ip1 = new Ip4Address("10.10.0.1");
            var ip2 = new Ip4Address("10.9.1.2");

            Assert.AreEqual(1, ip1.CompareTo(ip2));
        }

        [TestMethod]
        public void CompareTo_LessThan()
        {
            var ip1 = new Ip4Address("10.9.1.2");
            var ip2 = new Ip4Address("10.10.0.0");

            Assert.AreEqual(-1, ip1.CompareTo(ip2));
        }

        [TestMethod]
        public void CompareTo_Equal()
        {
            var ip1 = new Ip4Address("10.10.1.1");
            var ip2 = new Ip4Address("10.10.1.1");

            Assert.AreEqual(0, ip1.CompareTo(ip2));
        }
    }
}

Answer 1

Answering your questions.

1. Your GetHashCode() implementation is good enough.

2. It's better to store octets in the reverse order.
   The highest bits of an address should be stored in the Octet4, the lowest - in the Octet1.
   Thus you will be able to simplify your CompareTo method:

   public int CompareTo(Ip4Address other)
   {
       return Address.CompareTo(other.Address);
   }
   

3. And yes, this code depends on the endianness. If you are planning to use it on Big Endian systems too, you could use the BitConverter.IsLittleEndian static field to detect endianness and reverse octets if needed:

   var bytes = address.Split('.').Select(Byte.Parse);
   if (BitConverter.IsLittleEndian)
   {
       bytes = bytes.Reverse();
   }
   return bytes.ToArray();
   

   In this case you should also add a remark to descriptions of the Ip4Address(byte[] address) constructor and to the ToByteArray method, that the byte order depends on the endianness.

4. Your Equals(object obj) method can be simplified to:

   return obj is Ip4Address && this.Equals((Ip4Address)obj);
   

---

My vision of the ParseStringAddress static method:

private static byte[] ParseStringAddress(string address)
{
    if (address == null)
    {
        throw new ArgumentNullException(nameof(address));
    }

    string[] inputOctets = address.Split('.');

    const int expectedMaxLength = sizeof(uint);

    // Questionable.
    // Should we handle short forms of the IPv4, such as
    //   10.1.1
    //   192.168
    //   0
    // or not?
    // If not, rename the 'expectedMaxLength' back to the 'expectedLength'
    // and replace the '>' with the '!=' in the following condition:
    if (inputOctets.Length > expectedMaxLength)
    {
        throw new ArgumentException(...);
    }

    byte[] outputOctets = new byte[expectedMaxLength];
    for (int i = 0; i < inputOctets.Length; i++)
    {
        int outputOctetIndex = BitConverter.IsLittleEndian ? expectedMaxLength - 1 - i : i;
        if (!Byte.TryParse(inputOctets[i], out outputOctets[outputOctetIndex]))
        {
            throw new ArgumentException(...);
        }
    }

    return outputOctets;
}

In addition to the normal input strings it also accepts input strings:

• Questionable. With the short form of IPv4 address representing a subnet, such as 10.1.1 (equivalent of 10.1.1.0).
• Containing space chars, such as 1 . 2 . 3 . 4.

This method outputs octets in the right order depending on the BitConverter.IsLittleEndian value.

Answer 2

Consider adding a constructor Ip4Address(byte, byte, byte, byte) so that client code doesn't have to allocate a byte array or string, or manually convert the bytes to a uint.

---

ParseStringAddress accepts twice as many strings as there are IPv4 addresses. The reason is the behaviour of the $ anchor:

The match must occur at the end of the string or line, or before \n at the end of the string or line.

So new Ip4Address("0.0.0.0\n") will not throw an exception.

You can instead use the \z anchor:

The match must occur at the end of the string only.

I wouldn't go so far as to call this a bug, but it's good to be aware of. I consider the current behaviour to be surprising just because no other whitespace is allowed.