7
\$\begingroup\$

I was inspired by Processing a list to build an IP in String Format to reinvent the wheel a little bit and play around with explicit struct layouts.

You see, an IP address is really made up of four, 1-byte numbers. Each Octet of an IP address is one byte. So, this means that an IP address can be represented by either 4 bytes, or a single 32 bit unsigned integer. In other words, this is why the "largest" IPv4 address is 255.255.255.255.

255  | 255  | 255  | 255
0xFF | 0xFF | 0xFF | 0xFF

I've been looking for a reason to use the StructLayoutAttribute and this seemed like the perfect opportunity. I'm aware of the System.Net.IpAddress class, but I'm ignoring it (and IPv6) for the purpose of learning about memory layouts and data representation.

What can I do to make this a cleaner, more robust solution?

Note: I'm using C# v6 and leveraging string interpolation. This won't compile in earlier versions of the compiler.

Ip4Address

using System;
using System.Linq;
using System.Runtime.InteropServices;

namespace Rubberduck.Katas.Network
{
    [StructLayout(LayoutKind.Explicit)]
    public struct Ip4Address
    {
        [FieldOffset(0)] public byte Octet1;
        [FieldOffset(1)] public byte Octet2;
        [FieldOffset(2)] public byte Octet3;
        [FieldOffset(3)] public byte Octet4;

        /// <summary>
        /// Represents the Base Ten IPv4 address.
        /// </summary>
        /// <remarks>Overlays the Octet fields, so changing this value changes the Octets & vice versa.</remarks>
        // ReSharper disable once PrivateFieldCanBeConvertedToLocalVariable
        // ReSharper disable once FieldCanBeMadeReadOnly.Local
        [FieldOffset(0)] private UInt32 Address;

        /// <summary>
        /// Must be a valid IPv4 address.
        /// </summary>
        /// <param name="address"></param>
        public Ip4Address(string address)
            :this(address.Split('.').Select(a => Byte.Parse(a)).ToArray())
        { }

        /// <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)
            :this()
        {
            const int expectedLength = 4;

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

            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;
        }

        public override string ToString()
        {
            return $"{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]
        public void CanCreateFromByteArray()
        {
            Ip4Address ip = new Ip4Address(new byte[] {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 ByteArrayLengthCannotBeLessThan4()
        {
            var ip = new Ip4Address(new byte[] {});
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentException))]
        public void ByteArrayLengthCannotBeGreaterThan4()
        {
            var ip = 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());
        }
    }
}

Also available on GitHub.

\$\endgroup\$
3
  • \$\begingroup\$ new Ip4Address(null) throws NRE which is never allowed as a proper exception. That whole constructor needs to be revised. \$\endgroup\$ – usr Nov 28 '15 at 0:10
  • \$\begingroup\$ @usr I'd upvote an answer saying I need to throw a null art exception. hint hint; poke poke \$\endgroup\$ – RubberDuck Nov 28 '15 at 0:26
  • 1
    \$\begingroup\$ For reference, an IP address is made up of 32 bits. The splitting up into octets is for our convenience; to the computer, it's just one big number :P \$\endgroup\$ – cHao Nov 28 '15 at 2:55
3
\$\begingroup\$

First of all, your struct is mutable. It's a bad practice. Consider either to mark fields as readonly, or to make the fields private and to create public get-only properties.


Secondly, there is no input validation in the following constructor:

public Ip4Address(string address)
        :this(address.Split('.').Select(a => Byte.Parse(a)).ToArray())
{ }

I'd prefer to create a static method that inputs a string, validates it and returns a byte array:

private static byte[] ParseAddress(string address)
{
    // Validate and parse input here, throw exceptions.
}

And call it from the constructor above:

public Ip4Address(string address)
        : this(ParseAddress(address))
{ }

Minor issue.
When you call : this() in the constructor, it instructs the compiler to initialize each field with its default value. Later you override that value in the constructor's body. IMO, in some circumstances it makes sense to replace call to the default constructor with missing field(s) assignment. For instance:

public Ip4Address(byte[] address)
{
    const int expectedLength = 4;

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

    Address = 0; // Add this line instead of calling :this()

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

And the last.
Do you plan to use this struct as a collection key, compare two instances of the Ip4Address to each other?
If so, it makes sense to implement IEquatable<Ip4Address> and IComparable<Ip4Address> interfaces and override the GetHashCode method.


PS. I believe it's just a typo that the Address field isn't public visible. It makes no sense to use explicit struct layout and in the same time make overlapping fields private.

\$\endgroup\$
2
  • \$\begingroup\$ What benefit does IEquatable provide? I believe the default equality works well enough here. I could see implementing IComparable though. \$\endgroup\$ – RubberDuck Nov 27 '15 at 23:49
  • 1
    \$\begingroup\$ @RubberDuck Please see MSDN: "For a value type, you should always implement IEquatable<T> and override Object.Equals(Object) for better performance." and "If you implement IEquatable<T>, you should also implement IComparable<T> if instances of your type can be ordered or sorted. If your type implements IComparable<T>, you should also always implement IEquatable<T>." \$\endgroup\$ – Dmitry Nov 27 '15 at 23:57
2
\$\begingroup\$

You can leverage more C#6.0 features and change your ToString method up:

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

To:

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

I would recommend adding properties over all the fields.

I.e.:

[FieldOffset(0)] private byte _octet1;
public byte Octet1 => _octet1; // this is a get-only property

R# would also like me to remind you to use uint instead of UInt32.


You should probably mark these fields as readonly, since you only need to set them in constructors.


You can also make address.Split('.').Select(a => Byte.Parse(a)).ToArray() into a method group:

public Ip4Address(string address)
    : this(address.Split('.').Select(Byte.Parse).ToArray())
{ }

Overall, it looks quite good.

\$\endgroup\$
3
  • \$\begingroup\$ Would the explicit layout work on properties though?? ++ for the auto-bodies member though. =) \$\endgroup\$ – RubberDuck Nov 27 '15 at 21:50
  • \$\begingroup\$ No, but there's something for that too. \$\endgroup\$ – Der Kommissar Nov 27 '15 at 21:52
  • \$\begingroup\$ Nope. Can't specify the offset for properties, just fields. \$\endgroup\$ – RubberDuck Nov 27 '15 at 21:52
2
\$\begingroup\$

Missed test cases

What should happen here?

Ip4Address ip = new Ip4Address("123.456.789.999");

Or here:

Ip4Address ip = new Ip4Address("192.10.1.1.");

Or here:

Ip4Address ip = new Ip4Address("....");

Cause and effect

In this case, the numbers 192, 10, 1, 1 and the numbers in the asserts must correspond:

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

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

To make that connection explicit, I like to put such values into variables and use them in the inputs and the asserts. But maybe that's just me.

Exposing a byte array

These asserts feel a bit tedious:

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

How about exposing a byte[] of the octets, so that you could simplify some of the asserts with a single Assert on the byte[].

Empty is indeed less than 4 but...

This test case is named "less than 4", but inside it tests an empty byte[]:

[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void ByteArrayLengthCannotBeLessThan4()
{
    var ip = new Ip4Address(new byte[] {});
}

It's true that empty is less than 4, but empty is often special on its own right.

To emphasize more the aspect of "less than 4", I'd go with a test data of 3 octets instead of empty:

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

Local variables in test cases that throw

No need to store in a local variable the result of statements that are expected to throw:

[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void ByteArrayLengthCannotBeLessThan4()
{
    var ip = new Ip4Address(new byte[] {});
}

This is enough and simpler:

[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void ByteArrayLengthCannotBeLessThan4()
{
    new Ip4Address(new byte[] {});
}
\$\endgroup\$
1
  • \$\begingroup\$ Dang. I had meant to expose a ToByteArray() method... Good catch on the missing test cases too. \$\endgroup\$ – RubberDuck Nov 27 '15 at 22:13
2
\$\begingroup\$

new Ip4Address(null) throws NRE which is never allowed as a proper exception. That whole constructor needs to be revised because other uncontrolled exceptions are possible.

For performance reasons, address.Split('.').Select(a => Byte.Parse(a)).ToArray() is not ideal. Whether to keep that or not depends on what your performance needs are. Maybe this needs to become a helper method with a specialized loop doing this.

I believe you have a bug in that 1 . 2 . 3 . 4 is going to be allowed because some BCL parsing methods allow whitespace.

What about endianness issues? Does the overlaying of a uint on 4 bytes always work? I doubt it.

Ip4Address(byte[] address) throws NRE for a null input which is a bug, and untested.

CanCreateFromBaseTenAddress is misnamed because a unit is not base-10. It is not of any particular base. Maybe CanCreateFromInteger would be better.

I would put the two tests for length (ByteArrayLengthCannotBeLessThan4, ...) into one method and use an Assert.Throws method. There is little point in maintaining multiple tests for different length. Unit testing is no excuse for not having high quality code. Don't explode the code size by a factor of ~2x for no good reason.

\$\endgroup\$
0

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.