3

For the code below code, a hint is shown in sonar:

Refactor this code to use a "static final" Pattern.

I don't understand, what the issue is. It is not clearly explained in sonar, either.

Please help me.

public Boolean validateLabelText(String labelValue, String fieldCell) {
        return labelValue
               .replaceAll(StringUtils.SPACE, StringUtils.EMPTY)
               .equalsIgnoreCase(fieldCell.replaceAll(StringUtils.SPACE, StringUtils.EMPTY)
                                          .replaceFirst(StringUtils.LF, StringUtils.EMPTY));
    }
agabrys
  • 8,728
  • 3
  • 35
  • 73
Raj Kumar
  • 43
  • 1
  • 5

2 Answers2

5

Refactor this code to use a "static final" Pattern.

You hit the following issue: Regex patterns should not be created needlessly.

The problem is caused by these operations:

.replaceAll(StringUtils.SPACE, StringUtils.EMPTY)

.replaceFirst(StringUtils.LF, StringUtils.EMPTY)

Both methods create new instances of the java.util.regex.Pattern class. It means that when your method is executed 100 times, then your code creates 300 Pattern objects. The rule informs that you shouldn't do this, because Pattern are heavy objects and you can use more efficient code.

If StringUtils is org.apache.commons.lang3.StringUtils then the solution is as follow.

In the first operation

.replaceAll(StringUtils.SPACE, StringUtils.EMPTY)

you want to replace all occurrences of " " (space) by "" (empty). This code should be changed to:

.replace(StringUtils.SPACE, StringUtils.EMPTY)

The replace method replaces literals as is. replaceAll treats the first parameter as regular expressions. There is no sense to parse a space character as a regular expression, because the output will be exactly the same.

Next in

.replaceFirst(StringUtils.LF, StringUtils.EMPTY)

you always create a new Pattern object from StringUtils.LF. Instead of creating a new pattern every time, you should compile it once and next reuse it:

private static final Pattern LF_PATTERN = Pattern.compile(StringUtils.LF, Pattern.LITERAL);

LF_PATTERN.matcher(string).replaceFirst(replacement);

The final code should be as follow:

private static final Pattern LF_PATTERN = Pattern.compile(StringUtils.LF, Pattern.LITERAL);

public Boolean validateLabelText(String labelValue, String fieldCell) {
    String value1 = labelValue.replace(StringUtils.SPACE, StringUtils.EMPTY);

    String value2 = fieldCell.replace(StringUtils.SPACE, StringUtils.EMPTY);
    value2 = LF_PATTERN.matcher(value2).replaceFirst(StringUtils.EMPTY);

    return value1.equalsIgnoreCase(value2);
}

I don't know what is the business logic of your code, so I named variables as value1 and value2 (I don't know which value is current and which expected).

agabrys
  • 8,728
  • 3
  • 35
  • 73
0

rustyx ist right. The method could be made static final. I think, this is the cause of the sonar message. Also consider these three points.

replaceAll has a regular expression as its first parameter, but you do not need a regular expression here. You can use replace here. Use of regular expressions can reduce the performance, they are relatively slow.

Consider to use boolean as the return type. Using Boolean for a method that does a test is often bad style, because the caller has to check the return value for null also. There is rarely a need for a boolean to have a null value. When you use boolean, you say to the caller that the return value cannot be null and has not to be checked for that value. Using Boolean can also be slower than using boolean because of boxing/unboxing.

Btw: Do not use StringUtils.EMPTY. This constant is normally useless and only bloating the code. You can read this question and answers on the topic: Is StringUtils.EMPTY recommended? .

The performance issues (number 1 and 2) may not be really a problem, depending on the application and how often the method will be called.

Donat
  • 4,157
  • 3
  • 11
  • 26