Validating an IP address and returning the reason it's invalid

Question

I have a method that validates a user-written IP address in an Android application. If it's invalid, I need to know why and notify the user using a Toast.

I created an enum for this called Status that has a list of reasons for an IP address to be invalid and a value for when it's valid. The method isValid will return a Status value depending on whether or not it's invalid.

Android code

switch(IP.isValid(ip.getText().toString())) {
     case INVALID_NUMBER:
         Toast.makeText(MainActivity.this, "You have entered an invalid octect(s) value", Toast.LENGTH_SHORT).show();
         break;
     case INVALID_LENGTH:
         Toast.makeText(MainActivity.this, "You have entered an IP with an invalid length", Toast.LENGTH_SHORT).show();
         break;
     case NON_NUMERIC:
         Toast.makeText(MainActivity.this, "You have entered a non-numeric IP", Toast.LENGTH_SHORT).show();
         break;
}

• ip is an EditText field.

Status enum

public enum Status {
    INVALID_NUMBER,
    INVALID_LENGTH,
    NON_NUMERIC,
    VALID
}

isValid() method

public static Status isValid(String ip) {
    String[] octets = ip.split("\\.");
    if(octets.length != 4) {
        return Status.INVALID_LENGTH;
    }

    for(String octet : octets) {
        if(StringUtils.isNumeric(octet)) {
            int val = Integer.parseInt(octet);
            if(val > 255 || val < 0) {
                return Status.INVALID_NUMBER;
            }
        } else {
            return Status.NON_NUMERIC;
        }
    }

    return Status.VALID;
}

StringUtils.isNumeric()

(StringUtils is a class I created, it's not from Apache) https://stackoverflow.com/a/237204/4355066

public static boolean isNumeric(String str) {
    if (str == null) {
        return false;
    }
    int length = str.length();
    if (length == 0) {
        return false;
    }
    int i = 0;
    if (str.charAt(0) == '-') {
        if (length == 1) {
            return false;
        }
        i = 1;
    }
    for (; i < length; i++) {
        char c = str.charAt(i);
        if (c < '0' || c > '9') {
            return false;
        }
    }
    return true;
}

Is this the best way to this?

Answer 1

Your code has three major errors that all the previous reviewers failed to spot. If I were a lecturer, I'd show your code to the students as an example why they should always use libraries, even doing something seemingly simple as validating the IPv4 address.

• It accepts 127.0.0.1..... as a valid IP address, which it is not. Pass -1 as a second argument to the split function.

• Because of your faulty isNumeric function, the address 0.00123.0000034.12 is considered valid, too.

• Similar to above, the address 127.-0.-0.1 is accepted too.

Consider using a library for validating the IP addresses

As described above, you can never be too careful. Besides that, we are starting to make the migration to the IPv6 standard, which is harder to validate.

So, if you can rely on a well tested library for IP validation that does not bring an additional huge dependency to your code, use that instead.

It is not resilient against crafted inputs

Depending on the source of the input, the validation function might pose a huge performance risk and cause OM (out of memory) errors.

See, even really short strings of just dots ".........x" create one String object per character. String object and a pointer overhead in Java is rather in the neighborhood of 44 bytes. Therefore, someone sending you a 10 MiB string is typically going to overflow the heap space.

Code locality (tie error messages to enum values)

Because error strings are tied to enum values, it is a good thing to specify them in the actual enum construct.

public enum IpValidation {
    INVALID_NUMBER("You have entered an invalid octect(s) value"),
    INVALID_LENGTH("You have entered an IP with an invalid length"),
    NON_NUMERIC("You have entered an IP with an invalid length"),
    VALID("This IP address is valid");

    private final String error;

    private IpValidation(final String error) {
        this.error = error;
    }

    public String getError() {
        return error;
    }
}

Give the enum a more meaningful name

Strings is just a bad name to give an enum. Pick something else.

Nits

• Do put a space after the switch, if, and for keywords.
• Using enums on Android might have performance consequences. Depending on the use case, you might want to avoid it.

Answer 2

A few additional comments to complement the other review:

Avoid spelling errors, especially in user-visible strings

The first error message has "octect" but what was meant was undoubtedly "octet". Spelling errors in code are unfortunately relatively common, but we should all make special effort to avoid such errors in user-visible strings. It could lead a user of the code to wonder how many less visible errors are also in the code.

Convert variables only when required

The ip variable is already a String, so ip.getText().toString()) seems to be doubly redundant.

Consider other error conditions

Some addresses will pass this validation but may, depending on context, not be appropriate. Two such addresses are 0.0.0.0 and 255.255.255.255 (Broadcast address). Other special addresses might also be inappropriate, depending on context. It may be useful to augment the existing code with additional functions which check for usability within a given context.

Be aware of exceptions that may be thrown

The StringUtils.isNumeric(octet) call will return true with an empty string, but Integer.parseInt(octet) will throw a NumberFormatException. You may want to handle that in case the passed string is something like "0.0..0".

Answer 3

This is a very nice way to do it yes.

A couple of comments:

• The method performing the validation shouldn't be named isValid, because it does more than checking if the given IP adress is valid. If that were the case, it would only return a boolean true or false, saying whether IP given was valid or not. The method actually returns a status describing exactly what didn't validate. Consider renaming it to validate (a bit like the validate method of the Bean Validation API).
• Beware that there can be IPv4 and IPv6 addresses. Your code only handles the former case.
• If you use a switch to test the validation status, you could add a default case.