[Java] Don't use String.replaceAll(String, String) in a loop

Description

String.replaceAll(String, String) compiles its first argument as a regular expression every time it is called. This can use a significant amount of time when parsing files even if the expression is relatively simple. A better way is to use Pattern.compile() and store the result as a constant that will be reused.

Noncompliant code

public class MyClass {

    public void extractData(final InputStream stream, final String name) {

        try (InputStreamReader isr = new InputStreamReader(stream, StandardCharsets.UTF_8);
                        BufferedReader reader = new BufferedReader(isr)) {

            for (String line = reader.readLine(); line != null; line = reader.readLine()) {

                line.replaceAll(".", "");

            }

        } catch (IOException e) {
            // catch the IOException
        }
    }

}

Compliant code

public class MyClass {

    private static final Pattern pattern = Pattern.compile(".");

    public void extractData(final InputStream stream, final String name) {

        try (InputStreamReader isr = new InputStreamReader(stream, StandardCharsets.UTF_8);
                        BufferedReader reader = new BufferedReader(isr)) {

            for (String line = reader.readLine(); line != null; line = reader.readLine()) {

                pattern.matcher(line).replaceAll("");

            }

        } catch (IOException e) {
            // catch the IOException
        }
    }

}

External reference

Type

Code Smell

Tag

bad-practice

Kind regards,
Bryan

Hello @bcazabonne

First, welcome to the community, and thank you for taking the time to write this clear description.

I agree that there is a code smell in the noncompliant code, and in fact, when I analyze it, it is already reported by RSPEC-4248. It targets, amongst others, String.replaceAll and is more strict since it does not even require a loop to consider it as a bad pattern. In addition, your compliant code look exactly like the one of this rule.

At this point, I believe the case you described is already covered, am I right?

As a side note, there is also RSPEC-5361 targeting specifically replacAll (avoid it when the first argument is not a pattern).

Thank you for your answer. I think RSPEC-4248 can answer the problem. I will try to add that rule on my project.

Thank you again,
Bryan

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.