jsakamoto / ipaddressrange

.NET Class Library for range of IP address, both IPv4 and IPv6.
Mozilla Public License 2.0
368 stars 71 forks source link

Add ToCidrString() #21

Closed richardlawley closed 8 years ago

richardlawley commented 8 years ago

This PR adds a .ToCidrString() and .PrefixLength() which returns a Cidr notation form of a subnet, if the subnet exactly matches a range.

Examples:

IPAddressRange.Parse("192.168.0.0-192.168.0.255").PrefixLength(); // 24
IPAddressRange.Parse("192.168.0.0-192.168.0.255").ToCidrString(); // 192.168.0.0/24
IPAddressRange.Parse("fe80::-fe80:ffff:ffff:ffff:ffff:ffff:ffff:ffff").PrefixLength(); // 16
IPAddressRange.Parse("fe80::-fe80:ffff:ffff:ffff:ffff:ffff:ffff:ffff").ToCidrString(); // fe80::/16

If Begin and End do not align with any valid subnets, then FormatException is thrown.

jsakamoto commented 8 years ago

Thank you for your contribution!

But I'm busy in these days, so please let me a few days for merging & packaging & publishing.

jsakamoto commented 8 years ago

@richardlawley Why do you implement PrefixLength feature as a method, not a read only property?

I feel PrefixLength like a state of an IPAddressRange object, so I prefer that implement as a (read only) property.

richardlawley commented 8 years ago

Mostly because PrefixLength (and therefore ToCidrString) throws an exception if the range is not a CIDR range, e.g. 192.168.1.1-192.168.1.3. I'm not a fan of properties throwing exceptions.

If you want them as properties, then they'd need to be nullable instead.

jsakamoto commented 8 years ago

@richardlawley

I'm not a fan of properties throwing exceptions.

Oh, that's right. I'll feel the same way.

But the word of the member name "PrefixLength" is noun. I think a name of method should be verb.

Then I would like to suggest to you about changing the method name from "PrefixLength()" to "GetPrefixLength()".
How do you feel about my suggestion?

richardlawley commented 8 years ago

That's fine, I don't mind which. I can either change PrefixLength() to GetPrefixLength(), or I can make them into a nullable readonly property. Let me know which you prefer, and I'll update the PR.

jsakamoto commented 8 years ago

I prefer changing the name rather than nullable read only property. Thanks!

P.S.
Next, I'm considering about ToCidrString() method. This is good feature, but the another implementation style option, implement ToString() overload version.

For example, ToString("C") returns "192.168.0.0/24", and ToString("R") returns "192.168.0.0-192.168.0.255". ( In some case, ToString("C") throws FormatException )

How do you think this my idea?

richardlawley commented 8 years ago

Personally I dislike all of the "magic string" formats used by the various ToString overloads in Dates and numbers in the CLR, as I have to google them every time I want a new one! I think it would make much more sense to use an enum, e.g.

.ToString(IPRangeFormat.Cidr);     // 192.168.0.0/24
.ToString(IPRangeFormat.Range);    // 192.168.0.0-192.168.0.255
jsakamoto commented 8 years ago

@richardlawley

Personally I dislike all of the "magic string" formats used by the various ToString overloads in Dates and numbers in the CLR, as I have to google them every time I want a new one!

Haha! :smile:
I agree your sense.

Thank you for your proposal that enum version ToString() methods idea, but it doesn't have compatibility of the other standard .NET Framework libraries ( I haven't see enum argument version ToString() ).

So I prefer keep your original pull requst version - ToCidrString().

Ok, If you update your pull request about the changing name from "PrefixLength" to "GetPrefixLength", I'll merge it as soon as possible.

Thanks!

richardlawley commented 8 years ago

I've updated it now.

jsakamoto commented 8 years ago

I did it. http://www.nuget.org/packages/IPAddressRange/

Thank you!