What's the most effective way to refactor this simple method?

Question

I have implemented a very simple method:

private String getProfileName(String path) {
    String testPath = null;
    for (int i = 0; i < path.length(); i++) {
       testPath = path.substring(0, i);
          if ( testPath.endsWith("1") || testPath.endsWith("2") || testPath.endsWith("3") || testPath.endsWith("4") || testPath.endsWith("5") || testPath.endsWith("6") || testPath.endsWith("7") || testPath.endsWith("8") || testPath.endsWith("9") ) {
            break;
          }
    }
    return testPath.substring(0, (testPath.length() - 1));
}

I don't like the whole method because I think it's more complicated than necessary, especially the if condition.

So I thought of a way to refactor this method. First I thought of using Regex to replace the if condition, but isn't regex a little bit too much for this simple case?

Any other ideas how to reafctor this?

Answer 1

I'd say go with the regex. I can't think of a simpler way to refactor it. I can probably think of more complicated ways, but I don't think you'd want that.

Answer 2

Use this pattern with a matcher:

"^[^1-9]*"

Example code:

private String getProfileName(String path) {
    Pattern pattern = Pattern.compile("^[^1-9]*");
    Matcher matcher = pattern.matcher(path);
    matcher.find();
    return matcher.group();
}

I think this is much easier to understand than your code. It took me several minutes to work out your logic, and I had to run it to be sure. I think that with a regular expression it is quite clear what the code is doing. If you wish you can compile the regular expression just once and reuse it by moving it to a static member of the class.

Regular expressions have quite a bad stigma on Stack Overflow (mostly from people trying to use them to parse HTML, email addresses, URLs, and all sorts of other nasty inappropriate uses of regular expressions). But for this sort of task a regular expression is just fine.

You may also wish to consider why you are omitting 0 and if that is a good idea.

Answer 3

You can create a method like this to put inside your if

private static boolean endsWith1to9(String a) {
        for(int i = 1; i <= 9; i++) {
            if(a.endsWith(String.valueOf(i))) {
                return true;
            }
        }
        return false;
    }

Answer 4

Other than regexs:

private static String getProfileName(String path) {
    final int len = path.length();
    for (int i=0; i<len; ++i) {
       char c = path.charAt(i);
       if ('1' <= c && c <= '9') {
           return i==0 ? "" : path.substring(0, i-1); // Probably don't want that -1 !!
       }
    }
    return len==0 ? "" : path.substring(0, len-1);
}

Or perhaps, for Single Entry, Single Exit fans:

private static String getProfileName(String path) {
    final int len = path.length();
    int i=0;
    while (i != len && !isProfileTerminator(path.charAt(i))) {
    //Or (!(i == len || isProfileTerminator(path.charAt(i)))) {
       ++i;
    }
    return i==0 ? "" : path.substring(0, i-1);
}
private static boolean isProfileTerminator(char c) {
    return '1' <= c && c <= '9');
}

There are issues in the original code with strings that are empty or start with 1-9.

Answer 5

  private String getProfileName(String path)
  {
    int i;
    for (i = 0; i < path.length() && !Character.isDigit(path.charAt(i)); i++);
    return path.substring(0, i);
  }

Answer 6

I think something like this will work. Someone correct me if I'm wrong.

private String getProfileName(String path) {
    int i = 0;
    for(i = 0; i < path.length; ++i)
    {
        if(path.charAt(i) > '0' && path.charAt(i) <= '9') break;
    }
    return path.substring(0, i-1);
}

Answer 7

Mine:

private String getProfileName(String path) {

    return path.split("[1-9]")[0];
}

Hope this will help you.

Explanation. As Mark Byers said Split on digits (first version, second version ignores 0), and return the first element of the resulting array. But I think it won't fail if first argument is a digit (tested with jdk1.6.0_20). It fails if all characters from the path are digits (by example "2223"). You can use this version to avoid the error:

private String getProfileName(String path) {

    String[] result = path.split("[1-9]");
    return result.length > 0 ? result[0] : "";
}

As you will se at String's split method javadocs, it takes an argument (a regular expression), you can use one of this:

return path.split("[1-9]")[0]; //if you want to avoid 0
return path.split("\\d")[0]; //if you don't

IMHO: Using split method is better than other approaches if you are looking for improve your code readability,

Answer 8

Sure, you can refactor that using brute force...but why not use Apache Commons?

private String getProfileName(String path) {
    int index = StringUtils.indexOfAny(path, "123456789");
    if(index != -1) {
        return path.substring(0, index);
    }
    return path;
}

Answer 9

Here's a one-liner regex solution that is quite simple:

private String getProfileName(String path) {
    return path.replaceFirst("(?s)\\d.*", "");
}

The pattern is \d.* in Pattern.DOTALL mode, as embedded flag (?s). This will match a digit, and everything after it. We want to remove this part, so we replace with the empty string.

Note that \d includes 0 as well, so replace with [1-9] if that's really the specification.

References

• regular-expressions.info/Repetition, Dot, Character Class

Answer 10

My try:

/**
 * Numeric Register Optimized Search with Null Pointer Handling.
 * -> No use of Regex (per requirement). :)
 * -> No use of Character.isDigit() (NOTE: includes characters other than [0-9].)
 * -> Catch and Avoid NullPointerExceptions (Oops... review other methods above.)
 * -> Reduce for(...; test ; ...) to while(test) by variable declaration and ++off use.
 * -> Include ANDed test to avoid break call.
 * -> Cache "path.length" in local variable (e.g. CPU register)
 * -> Cache "path.charAt(off)" in local variable (e.g. CPU register)
 * -> Replace String.endsWith(...) Method with char < char Register Method.
 * -> Reuse path argument to avoid another internal object reference.
 * -> Avoid call to substring if [1-9] not found in path.
 * -> Single Entry/Single Exit Happy. :)
 * @return path null if passed null, otherwise valid value.
 */
private String getProfileName(String path) {
    if (path != null) {
        int off = -1, len = path.length;
        final char one = '1', nine = '9';
        char c;
        while (++off < len && (c = path.charAt(off)) < one && c > nine);
        if (off < len) {
            path = path.substring(0, off);
        }
    }
    return (path);
}

Cheers, loeJava