0
\$\begingroup\$

I am validating two IP addresses. I simply wrote a peice of code like

if(string.IsNullOrEmpty(ip1) && string.IsNullOrEmpty(ip2) 
  return true;

But is this logically correct? What should we done with empty strings? If both are empty strings, is it okay to return true?

May be a complete code snipet can give better insight.

public static bool IsIPAddressMatching(string ipAddressA, string ipAddressB)
        {
            try
            {
                if(string.IsNullOrEmpty(ipAddressA) || string.IsNullOrEmpty(ipAddressB))
                    return true;
                var address1 = IPAddress.Parse(ipAddressA);
                var address2 = IPAddress.Parse(ipAddressB);

                if (address1.AddressFamily == address2.AddressFamily)
                    return (address1.Equals(address2));

               // some code here to check mapped IP addresses

            }
            catch (Exception)
            {
                return false;
            }
        }
\$\endgroup\$
  • 2
    \$\begingroup\$ What do you want to achieve? \$\endgroup\$ – Thomas Junk May 14 '13 at 7:43
  • \$\begingroup\$ It seems not clear enough to give an answer \$\endgroup\$ – Chamika Sandamal May 14 '13 at 7:50
  • 1
    \$\begingroup\$ You don't need to check AddressFamily yourself, IPAddress.Equals() already does that. \$\endgroup\$ – svick May 14 '13 at 11:29
2
\$\begingroup\$

From the IP validation point of view null or empty IP addresses are exceptional actions so you can not say that two null or empty strings are the same IP addresses becouse they aren't IP addresses and sometimes not even strings.

And you don't have to sorrund the logic with try-catch block becouse it will eat usefull information when things will get ugly. If you don't want to deal with parsing exception then use IPAddress.TryParse it will be faster becouse throwing an exception is always a heavy weight stuff (check it with profiler).

public static bool IsIPAddressMatching(string ipAddressA, string ipAddressB)
{
    if (ipAddressA == null)
    {
        throw new ArgumentNullException("ipAddressA");
    }
    if (ipAddressB == null)
    {
        throw new ArgumentNullException("ipAddressB");
    }

    if (string.IsNullOrEmpty(ipAddressA))
    {
        throw new ArgumentException("ipAddressA cannot be an empty string", "ipAddressA");
    }
    if (string.IsNullOrEmpty(ipAddressB))
    {
        throw new ArgumentException("ipAddressB cannot be an empty string", "ipAddressB");
    }

    var address1 = IPAddress.Parse(ipAddressA);
    var address2 = IPAddress.Parse(ipAddressB);

    if (address1.AddressFamily == address2.AddressFamily)
    {
        return (address1.Equals(address2));
    }

   // some code here to check mapped IP addresses

   return ...
}
\$\endgroup\$
  • \$\begingroup\$ What's the point in the first 4 if (...) throw...s? The IPAddress.Parse method will throw an ArgumentNullException if either ipAddressA or ipAddressB are null. It will also throw a FormatException if the string is not a valid IP address. I don't think all the checks first add anything. \$\endgroup\$ – RobH May 14 '13 at 14:49
  • \$\begingroup\$ +1 for the ArgumentNullException part. Mostly C/C++ programmers tends to forget about this :) \$\endgroup\$ – sarat May 14 '13 at 14:56
1
\$\begingroup\$

Personally i would check the strings on NullOrWhitespace, but knowing you want to validate IP addresses, i recommend to use the IPAddress object for them which has an Parse and TryParse method. When that's done it's quite easy to create an static method for it which provides easy access.

Below i have written an example:

References:

using System;
using System.Linq;
using System.Net;

Code:

public static class IPAddressValidation
{
    public static bool Validate(string self)
    {
        IPAddress result;
        IPAddress.TryParse(self, out result);

        return result != null;
    }

    public static bool Validate(string[] self)
    {
        return self.All(Validate);
    }
}

Also, in some cases it might be sufficient to just throw an exception, when you are expecting it to be always valid for example, instead of user input. Validation methods don't always have to return true or false, sometimes it's such an essential value, that stopping the whole process by throwing the exception might be desired behavior.

\$\endgroup\$
  • 1
    \$\begingroup\$ I agree entirely with using the inbuilt type IPAddress to attempt to parse the string but this is a horrible implementation. TryParse returns a bool indicating its success so you shouldn't be checking for null. I also don't think you should have this as an extension method - especially one called 'IsValid': string a = "123"; bool result = a.IsValid() - Is a valid what? \$\endgroup\$ – RobH May 14 '13 at 8:07
  • \$\begingroup\$ Yes i agree on the implementation, updating it in a second, still i would check on null though, but that's my personal preference i guess. \$\endgroup\$ – Viezevingertjes May 14 '13 at 8:14
  • \$\begingroup\$ Point being your return statement in Validate could be : return IPAddress.TryParse(self, out Result); \$\endgroup\$ – Reacher Gilt May 15 '13 at 21:14
  • \$\begingroup\$ Yes i know what he was saying, but then, what would be the use of 'Result', i think this is better for the readability as every programmer now knows what's happening, to some it would make no sense to fill 'Result', while returning on the same line for example. \$\endgroup\$ – Viezevingertjes May 16 '13 at 10:10

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.