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?

• Sumurai8: I have proposed an edit to revert your change, because you changed the code after you already received reviews. See What you may and may not do after receiving answers for more information.
• 200_success: @Sumurai8 Adding contextual information to clarify the question is fine.
• Sumurai8: It still invalidates Edward's answer in a way, but alas. We'll keep it this way.
• Sumurai8: Also @Vincent. You seem to have reverted 200's tag edits.
• 200_success: @Sumurai8 Edward had made a wrong assumption. Clarification is not invalidation — there is no intention in Rev 3 to change the question. Correcting wrong assumptions happens all the time on Stack Exchange sites. It's fine.

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.

• Roland Illig: As soon as the app gets internationalized, tying the error message to the enum becomes problematic.
• Rok Kralj: Depending on the internationalization method, this might pose a problem, yes.
• Tunaki: Technically, one could say that accepting 127.0.0.1..... or 0.00123.0000034.12 as input is a benefit to the caller because it makes the validation less strict while still correct (so more user-friendly). Then it depends how the rest of the code uses it (if it expects a strictly valid IP address or not). Still, you're right that it's a good point to make.
• Rok Kralj: I disagre. Allowing empty spaces after and before would be a much more user friendly option, but it is still rejected by the function. Being strict is a good thing. Besides, always consider possible future security concerns. Let's always be strict.
• ilkkachu: Should 0.00123.0000034.12 be invalid? And why? Should also 127.000.000.001 be invalid? (note that allowing zeroes in the front might be useful if someone wants to present the addresses as fixed-width strings, and it doesn't change the numerical value of decimal numbers).

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".

• Tunaki: Curious, how do you know ip is already a String? I don't see the declaration of this variable in the given code (and assumed it was an Android component). Also worth noting that it seems isNumeric was changed in Commons Lang 3.0 to return false on empty String.
• vincentes: ip is an EditText field. I'm using the isNumeric() method from Jona's answer in stackoverflow.com/questions/237159/… with my own StringUtils. I should have specified this in the question, sorry about that.
• Edward: @Vincent: Ah, that makes sense, so in that case those items won't necessarily apply to your code. With that said, though, I'll leave them in my answer in case they might be of use to future readers.

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.