1
\$\begingroup\$

For legacy reason (guys here LOVE immutable object), I need a struct that contain an IpV4.

I can't use System.Net.IPAddress because it's not a struct.

So far here what I got:

public struct IpV4Address
{
    readonly byte[] address;

    public IpV4Address(byte[] address)
    {
        Throw.IfArgumentIsNull("address", address);

        if (address.Length != 4)
        {
            throw new ArgumentException("Wrong address length", "address");
        }

        this.address = new byte[4];
        Array.Copy(address, this.address, 4);
    }

    public override string ToString()
    {
        return string.Join(".", address);
    }

    /// <summary>
    ///     Parse a string representing an IPV4.
    ///     <example>xxx.yyy.zzz.www</example>
    /// </summary>
    /// <param name="quadDotted">The string to parse.</param>
    /// <returns>The IPV4</returns>
    public static IpV4Address Parse(string quadDotted)
    {
        Throw.IfArgumentIsNull("quadDotted", quadDotted);

        var bytes = quadDotted.Split('.').Select(byte.Parse).ToArray();

        return new IpV4Address(bytes);
    }

    /// <summary>
    ///     Try parse a string representing an IPV4.
    ///     <example>xxx.yyy.zzz.www</example>
    /// </summary>
    /// <param name="quadDotted">The string to parse.</param>
    /// <param name="result">The IPV4</param>
    /// <returns>A value indicating whether the parse succeed.</returns>
    public static bool TryParse(string quadDotted, out IpV4Address result)
    {
        if (quadDotted == null)
        {
            goto Fail;
        }

        var elements = quadDotted.Split('.');
        if (elements.Length != 4)
        {
            goto Fail;
        }

        var address = new byte[4];
        for (var i = 0; i < 4; i++)
        {
            if (!byte.TryParse(elements[i], out address[i]))
            {
                goto Fail;
            }
        }

        result = new IpV4Address(address);
        return true;

        Fail:
        result = default(IpV4Address);
        return false;
    }

It's the first time I use a goto in a C# code. And I don't feel ashamed !

\$\endgroup\$
  • \$\begingroup\$ Hi there! What kind of review would you like for your code? Readability, performance, something else? \$\endgroup\$ – SuperBiasedMan Oct 1 '15 at 10:00
  • 1
    \$\begingroup\$ @SuperBiasedMan, maybe Readability, I need a second advice for the TryParse method. \$\endgroup\$ – Orace Oct 1 '15 at 10:05
  • \$\begingroup\$ goto?? Don't you dare!!! (I've got nothing to post, answers here are good already) \$\endgroup\$ – IEatBagels Oct 2 '15 at 13:06
6
\$\begingroup\$

I find it more readable if you write the method like the following:

static bool TryParse(string quadDotted, out IpV4Address result)
{
    result = default(IpV4Address);

    if (quadDotted == null)
    {
        return false;
    }

    var elements = quadDotted.Split('.');
    if (elements.Length != 4)
    {
        return false;
    }

    var address = new byte[4];
    for (var i = 0; i < 4; i++)
    {
        if (!byte.TryParse(elements[i], out address[i]))
        {
            return false;
        }
    }

    result = new IpV4Address(address);
    return true;
}

or:

static bool TryParse(string quadDotted, out IpV4Address result)
{
    bool canParse = false; 

    if (quadDotted != null)
    {
        var elements = quadDotted.Split('.');
        if (elements.Length == 4)
        {
            var address = new byte[4];

            // the following part can also be transformed in a do-while cycle
            canParse = byte.TryParse(elements[0], out address[0]);
            for (var i = 1; i < 4 && canParse; i++)
            {
                canParse = byte.TryParse(elements[i], out address[i]);
            }
        }
    }

    result = canParse ? new IpV4Address(address) : default(IpV4Address);
    return canParse;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The double result assignation make me uncomfortable. Is it side effects free ? \$\endgroup\$ – Orace Oct 1 '15 at 10:13
  • \$\begingroup\$ What do you mean by 'side effects' in this case? It's just creating an object/struct and assigning it to a variable, I don't see any side effects. Anyway, I modified the code so the assignment is done only once (I didn't test the second version though). \$\endgroup\$ – Gentian Kasa Oct 1 '15 at 10:40
  • \$\begingroup\$ I like the second version. It fix my uncomfortable status ;) \$\endgroup\$ – Orace Oct 1 '15 at 14:38
  • \$\begingroup\$ Assigning a variable twice is less efficient use of processing, but kangaroo code is not easy to maintain. \$\endgroup\$ – Code Gorilla Oct 2 '15 at 12:51
5
\$\begingroup\$

ToString()

In its current form this method will throw a SystemArgumentNullException if you use for instance this

IpV4Address adress = default(IpV4Address);
Console.WriteLine(adress);

you can either return base.ToString() or just return "0.0.0.0" if address == null like so

public override string ToString()
{
    if (address == null) { return "0.0.0.0"; }

    return string.Join(".", address);
}  

another maybe better way would be to use the null coalescing operator ?? like so

public override string ToString()
{
    return string.Join(".", address ?? new byte[4]);
}  

For the TryParse() you should go with @GentianKasa's version with a little polish

  • quadDotted isn't really a good name and it isn't needed that you tell the caller that the passed in string should be dotted or quadDotted because at least it would only need 3 dots. So either a simple value or ip4Address would do.

Some nitpicks

The constructor should throw an ArgumentOutOfRangeException instead of an ArgumentException.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Just a thought: the problem with the ToString() method can be avoided if readonly byte[] address; is changed in readonly byte[] address = new byte[4]; and in the constructor you just copy the values of the param instead of assigning the array to address. \$\endgroup\$ – Gentian Kasa Oct 1 '15 at 11:27
  • \$\begingroup\$ Yeah, you're right. Forgot that structs are different than classes from this point of view. \$\endgroup\$ – Gentian Kasa Oct 1 '15 at 12:04
3
\$\begingroup\$
  1. In the Parse function you need to check that there are 3 dots in the address that are not adjacent in the string.
  2. You could also do with checking that the values in between the dots are positive (or 0) and less than 256.
  3. As said above it would be better if you throw an exception from the TryParse function rather than use an out or gotos. (although I now see why it returns bool, doh!)
  4. Why cant the TryParse function use the Parse function and just catch the exceptions?
  5. Try parse is testing for a null string but not a null/whitespace/empty string.

The code looks like it was written by two people, the styles are quite different.

I spotted an issue with the scope of address and I removed the body of the loop, it think you might be able to foreach the loop.

    static bool TryParse(string quadDotted, out IpV4Address result)
    {
        if (!String.IsNullOrWhiteSpace(quadDotted))
        {
            String[] elements = quadDotted.Split('.');
            if (elements.Length == cMaxBytes)
            {
                byte[] address = new byte[cMaxBytes];
                for (var i = 0; i < cMaxBytes && byte.TryParse(elements[i], out address[i]); i++)
                {
                }
                result = new IpV4Address(address);
                return true;
            }
        }
        result = default(IpV4Address);
        return false;
    }
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ 1. will be covered by the constructor. 2. is done by byte.Parse respectively TryParse. 3. a TryParse method shouldn't throw by definition. 4. is a real dirty way ( I had suggested it too, but later deleted it because we want to make the code shine). 5. this is absolutely correct and deserves my +1 \$\endgroup\$ – Heslacher Oct 2 '15 at 12:56
  • \$\begingroup\$ 1. byte.TryParse will fail with null/empty string. 2. byte.TryParse will fail with value outside of [0..255] 3. A try .. catch costs way more than a goto. TryParse is a Try function .. that it. It return a bool has it's done for a lot of TryParse function in c#. 5. If the entry is null, the split call will throw. It's not the case for whitespace/empty string (in those cases, it's the byte.TryParse that return false). \$\endgroup\$ – Orace Oct 2 '15 at 12:56
  • \$\begingroup\$ @Heslacher, empty/whitesapce string will lead to an empty elements array. \$\endgroup\$ – Orace Oct 2 '15 at 12:59
  • \$\begingroup\$ if byte.TryParse does not succeed, the address[i] still is valid. cMaxBytes is a good idea, but you have to declare it somewhere ;-) \$\endgroup\$ – Heslacher Oct 2 '15 at 13:05
  • \$\begingroup\$ @Heslacher - I stand corrected on 1 to 4 :( I know (about cMaxBytes) it was at the top of the file :) Its 4 if that helps :) \$\endgroup\$ – Code Gorilla Oct 2 '15 at 13:20

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.