patterncsharpMinor
IPv4 struct utilizing explicit layout
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
I've been looking for a reason to use the StructLayoutAttribute and this seemed like the perfect opportunity. I'm aware of the
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.
///
///
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 | 0xFFI'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
Secondly, there is no input validation in the following constructor:
I'd prefer to create a static method that inputs a string, validates it and returns a byte array:
And call it from the constructor above:
Minor issue.
When you call
And the last.
Do you plan to use this struct as a collection key, compare two instances of the
If so, it makes sense to implement
PS. I believe it's just a typo that the
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.