Java : Proper way a method() should handle a bad call

Question

I am often confronted to this question. Let's see a quick example, I got this simple method : It retrieves the position of a String in a String[].

public int getStringPosition(String s, String[] text) {
    for (int i = 0; i < text.length; i++) {
        if (text[i].equals(s)) {
            return i;
        }
    }
    return text.length;
}

But what if the String isn't in the array? Here, it returns the size of the array (an int that couldn't come from a normal use of the method). It could as well return -1. I wish it would return null but that doesn't seem to be possible.

Anyway, this method will be used in the same class by some other methods. My question is pretty general, how to handle the cases where you can't return what is expected?

Answer 1

If the inability to find the string constitutes an error, an exceptional condition, then you should throw some exception to the caller. E.g.,

throw new RuntimeException("String not found");

If the inability to find the string is a normal situation that the caller would want to handle in the normal flow of the code, then you should use a special return value. For this type of search method, no normal return value could be negative, and the convention is to return -1, as String.indexOf and List.indexOf do. If you follow that convention you could actually implement your method body with a single line:

return java.util.Arrays.asList(text).indexOf(s);

Note: That change treats null a bit differently. There are three things that could be null in your method: the search string, the array, and the elements of the array. Your current code throws a NullPointerException if passed a null array, or when it encounters a null element, but silently ignores a null search string. By deferring to List.indexOf, all the array elements are allowed to be null too, allowing for searching for a null value within the array. It's probably a harmless difference here, but a well-designed method considers such questions. For example, you might prefer to treat a null search string as invalid input and immediately throw an exception in that case.

A fully documented public method would also consider the case of multiple matching elements within the array: should it return the lowest matching index, or any matching index? If any index is acceptable, must it consistently return the same index for repeat invocations with the same arguments? (A hypothetical multithreaded search implementation could easily defeat the "obvious" assumption of returning the lowest index, because you can't predict which CPU core will find a match first.)

Also consider that the method could be declared static.

Answer 2

If the case that the s is not in text is not a valid way to call the method, you should throw an Exception e.g. IllegalParameterException.

But if it is ok to call the method this way, you should not throw an Exception because it is a valid expected usage. In this case you could e.g. return -1.

But -1 is in this case a 'magic number' which is kind of bad practice so you should declare a constant for that.

private static final int STRING_NOT_IN_TEXT=-1;

/**
 * @return first position of 's' in 'text' 
 *         and if 's' is not in 'text' returns NOT_IN_TEXT
 */
public static int getStringPosition(final String s, final String[] text) {
    for (int i = 0; i < text.length; i++) {
        if (text[i].equals(s)) {
            return i;
        }
    }
    return STRING_NOT_IN_TEXT;
}

P.s. You may also want to check if the text is null and handle this case other than simply let the system throw a NullPointerReference.