IPAddress constructors and Parsing improvements

[gregmac]

Adds new constructors so it's possible to pass in a System.Net.IPAddress object. If you already have instantized IPAddress objects, it's silly to have to call .ToString() and then IPAddressRange.Parse().

New constructors:

    /// <summary>
    /// Creates an empty range object, equivalent to "0.0.0.0/0".
    /// </summary>
    public IPAddressRange() 

    /// <summary>
    /// Creates a new range with the same start/end address (range of one)
    /// </summary>
    /// <param name="singleAddress"></param>
    public IPAddressRange(IPAddress singleAddress)

    /// <summary>
    /// Create a new range based on the first and last IPs in an array.
    /// Throws an exception if the array is empty or null. All but the first and last values 
    /// are ignored. Passing a single value is equivalent to a range of one address.
    /// </summary>
    /// <param name="beginEnd"></param>
    public IPAddressRange(ICollection<IPAddress> beginEnd)

    /// <summary>
    /// Creates a range from a base address and mask bits 
    /// </summary>
    /// <param name="baseAddress"></param>
    /// <param name="maskLength"></param>
    public IPAddressRange(IPAddress baseAddress, int maskLength)

    /// <summary>
    /// Creates a range from a base address and subnet mask.
    /// </summary>
    /// <param name="baseAddress"></param>
    /// <param name="subnetMask"></param>
    public IPAddressRange(IPAddress baseAddress, IPAddress subnetMask)

I also refactored Parse() to call the above constructors, and then went a step further and collapsed it down into a single Regex, which is a bit more efficient and clean (related #9).

[gregmac]

I found adding test cases to parsing incredibly tedious, so because AssertEx was already there an extending MSTest I used TestCaseAttribute to build parameterized tests. All existing parsing tests were converted to TestCases and several new ones were added.

FWIW the only new bug I found in existing code was #14. I did find two bugs in my new parsing code (63896a3, 8e4677f) that weren't covered by original test cases, which is good reason why full coverage is important.

This would be cleaner with basically any other test framework (eg, NUnit or xUnit) but I added Console logging so you can at least see which case is failing (in the test output) when running.

[jsakamoto]

Thank you for your suggestion.

If you already have instantized IPAddress objects, it's silly to have to call .ToString() and then IPAddressRange.Parse().

Yes, you are right, I think too, I agree.

But some constructor override versions feel like not beautiful for me.

1. Default constructor (no arguments version) is realy needed? I want to drop no arguments version constructor, because I feel it is good at nothing.
2. I think, the syntax of IPAddressRange(ICollection<IPAddress>) is not developer friendly.This syntax does not describe that the constructor expect two IPAdress. And, it can not detect errors at compile time. I would like to IPAddressRange(IPAddress begin, IPAddress end), but, in your design, it is conflict the constructor version of IPAddressRange(IPAddress baseAddress, IPAddress subnetMask). Is there any good idea?

[gregmac]

1. You had the default constructor there already, so I left it that way. A default constructor is necessary for serialization (though I think JSON.NET at least can deserialize even if that constructor is protected). It's also useful to enable the syntax var obj = new IPAddressRange() { Begin = rangeBegin, End = rangeEnd };
2. I had a hard time with the range vs ip+subnetmask constructor.

Some alternative options:

• Use Tuple instead of Collection: public IPAddressRange(Tuple<IPAddress,IPAddress> beginEnd)

  • This was actually my first approach, but it's harder to call, eg: new IPAddressRange(new Tuple<IPAddress,IPAddress>(rangeBegin, rangeEnd)) (vs with Collection: new IPAddressRange(new[] {rangeBegin, rangeEnd }).)

• Use a parameter to specify ctor type:

    public Enum AddressType { Range, Subnetmask }
    public IPAddressRange(IPAddress addr1, IPAddress addr2, AddressType addressType)
  

  Of course, you'd have to specify this when calling the constructor, and it's strange that it's inconsistent from the rest. We could at least make it an optional param, but then usage may be confusing.

• Pick one, and create a public static IPAddressRange FromSubnetmask(IPAddress, IPAddress) or public static IPAddressRange FromRange(IPAddress, IPAddress)

  • The inconsistency here is annoying though

• Drop all constructors, and change all the ones I made to be static methods

  • Personally I'm not a huge fan of this, but sometimes it's appropriate

• Make a new constructor that takes maskbits, and a static conversion method:

    public IPAddressRange(IPAddress baseAddress, int maskBits)
    public static int SubnetMaskBits(IPAddress subnetMask) 
  

  This would make invocation: new IPAddressRange(myIp, IPAddressRange.SubnetMaskBits(mySubnetMask)). Actually not a terrible option: adds a new constructor, and maybe makes it a bit more obvious to tell if an IPAddress is valid as a subnet mask (eg it's that method throwing an exception vs the constructor).

• Leave it off, and suggest the user uses the syntax: new IPAddressRange() { Begin = rangeBegin, End = rangeEnd }

I kind of thought that the array was a good balance... kept constructors and still easy to call. I still don't mind it, but I also don't mind doing the last two options I put above: subnetmask-to-bits conversion or just leaving it off entirely.

[jsakamoto]

You had the default constructor there already, so I left it that way. A default constructor is necessary for serialization

Oops... it's my mistake. 😵
OK, we need default constructor, at least support serialization.

I had a hard time with the range vs ip+subnetmask constructor.
Some alternative options:

I would like to chose "Make a new constructor that takes maskbits, and a static conversion method" version.

But, please keep in mind about IPAddressRange(IPAddress baseAddress, int maskBits) and IPAddressRange(IPAddress baseAddress, int maskLength) is same syntax.

How to resolve the conflict of constructor syntax?
Should we introduce new SubnetMask class?

[gregmac]

But, please keep in mind about IPAddressRange(IPAddress baseAddress, int maskBits) and IPAddressRange(IPAddress baseAddress, int maskLength) is same syntax.

My mistake, those are same things.

How to resolve the conflict of constructor syntax?
Should we introduce new SubnetMask class?

There would be no conflict .. am I missing something?

So desired changes:

• Remove public IPAddressRange(IPAddress baseAddress, IPAddress subnetMask)
• Change public IPAddressRange(ICollection<IPAddress> beginEnd) to public IPAddressRange(IPAddress begin, IPAddress end)
• Add public static int SubnetMaskLength(IPAddress subnetMask) (and tests)
• Update XML help for public IPAddressRange(IPAddress baseAddress, int maskLength) to show example use of SubnetMaskLength()

Did I get that right?

After this the following would work:

var begin = IPAddress.Parse("10.0.0.2");

var end1 = IPAddress.Parse("10.0.0.29");
var range1 = new IPAddressRange(begin, end1);

var subnetMask2 = IPAddress.Parse("255.255.255.0");
var subnet2 = new IPAddressRange(begin, SubnetMaskLength(subnetMask2));

[jsakamoto]

OK, I feel those constructors sets are clear, beautiful, and developer friendly 👍

Can I expect about you will resend pull request include those constructors changing?

[gregmac]

Yeah, I will update this PR with those changes.

[jsakamoto]

Well, I'm looking forward to your updating of this pull request.