HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

IPv4 struct utilizing explicit layout

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
structutilizingipv4explicitlayout

Problem

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;

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

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

///
/// Creates a new Ip4Address from a byte array.
///
///

Solution

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 and IComparable 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.

Code Snippets

public Ip4Address(string address)
        :this(address.Split('.').Select(a => Byte.Parse(a)).ToArray())
{ }
private static byte[] ParseAddress(string address)
{
    // Validate and parse input here, throw exceptions.
}
public Ip4Address(string address)
        : this(ParseAddress(address))
{ }
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];
}

Context

StackExchange Code Review Q#112088, answer score: 3

Revisions (0)

No revisions yet.