[java] New rule for simplifiable system property access

[oowekyala]

Rule name: TBD

Add a rule to convert calls to System.getProperty("line.separator") to use System.lineSeparator() since that is the reason the method was added See JDK-6900043.

Originally posted by @Sthornton12 in #1993 (comment)

See also File.separator/ File.separatorChar vs System.getProperty("file.separator"), although those don't have an @since

[linusjf]

UsePerformantProperties if you're using the rule for both properties. UsePerformantLineSeparator and UsePerformantFileSeparator otherwise. Or just PerformantXXX.

[oowekyala]

I don't think this is performance-related at all... Mostly this is about verbosity and security, as the JDK bug report explains. Maybe UseSystemPropertyShorthands?

[linusjf]

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6264243

Refer above.

Implementation of lineSeparator() in OpenJDK 7:

    public static String lineSeparator() {
        return lineSeparator;
    }

    private static String lineSeparator;

http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/cf44386c8fe3/src/share/classes/java/lang/System.java

[oowekyala]

Thanks for linking this ticket. I get that the performance is likely better. But I still think the performance concern should be secondary and not in the name of the rule. The other concerns seem more universal (Correctness/Ease of use & cross-platform development/Readibility/Security), whereas "Performance" depends on JDK implementation, and how hot the code is.

[linusjf]

How about DeprecateSystemPropertyCalls or ReplaceSystemPropertyCalls? No matter what you name rules, it's quite unlikely that users 'get it' until they read the description of what it actually does. I have no idea if the Oracle JDK implementation differs. I don't see why it would. https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#lineSeparator() This is the existing contract for the API. (My feeling is that this is inconsistent. Either the security manager should disallow system properties from being updated or that a property change listener ought to update this field. The only way there is consistency is if the property is specified from the command line. Or did the API designers feel that letting the developers update this field once it's been set would lead to inconsistency? I wouldn't like to second-guess them. ) https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#getProperties() https://stackoverflow.com/questions/22358071/differences-between-oracle-jdk-and-openjdk/38685948#38685948 As of Java 11, the two versions are converging as per the above post. You have StringBuffer and StringBuilder rules. Don't those improve performance? Similarly, deprecation of Vector usage. You already have a category for Performance/Optimization. https://pmd.github.io/latest/pmd_rules_java_performance.html Another post: http://marxsoftware.blogspot.com/2018/02/system-line-separator-jdk7.html?m=1

[oowekyala]

How about DeprecateSystemPropertyCalls or ReplaceSystemPropertyCalls?

The second one seems very fine to me.

You have StringBuffer and StringBuilder rules. Don't those improve performance? Similarly, deprecation of Vector usage.

Yes, those do improve performance and that's about all they do.

By contrast, this rule's most important contribution is IMO not improving performance. That's just a happy side effect, that in most cases will be next to irrelevant.

[linusjf]

This is correct only if the property is set at start-up. Have you tried setting it from the command line? Any cached value gives better performance. Are you expecting no collisions for this key? Then, yes, it's pretty much the same. Run it a million times and you might see a difference. A few additional function calls internal to Properties object won't slow it down substantially. It's unlikely that any program will invoke getProperty a million times in sequence. The performance aspect of this rule is negligent. On giving it further thought, I'd call the rule UseLineSeparatorCall since you'd hardly replace all system property calls , just this specific one. "A number of classes in the base module use System.getProperty("line.separator") and could use the more efficient System.lineSeparator() to simplify the code and improve performance." https://dzone.com/articles/prefer-systemlineseparator-for-writing-system-depe Frankly, it's more important to have the rule than a debate whether the rule is to be in the performance/optimization category or another. As long as it's in. A workable compromise would be to have the rule in both categories: Performance and Correctness. PMD already checks for multiple instances of a rule.