rspec/rules/S2629/java/rule.adoc

== Why is this an issue?

Passing message arguments that require further evaluation into a Guava ``++com.google.common.base.Preconditions++`` check can result in a performance penalty. That's because whether or not they're needed, each argument must be resolved before the method is actually called.


Similarly, passing concatenated strings into a logging method can also incur a needless performance hit because the concatenation will be performed every time the method is called, whether or not the log level is low enough to show the message.


Instead, you should structure your code to pass static or pre-computed values into ``++Preconditions++`` conditions check and logging calls.


Specifically, the built-in string formatting should be used instead of string concatenation, and if the message is the result of a method call, then ``++Preconditions++`` should be skipped altogether, and the relevant exception should be conditionally thrown instead.


=== Noncompliant code example

[source,java]
----
logger.log(Level.DEBUG, "Something went wrong: " + message);  // Noncompliant; string concatenation performed even when log level too high to show DEBUG messages

logger.fine("An exception occurred with message: " + message); // Noncompliant

LOG.error("Unable to open file " + csvPath, e);  // Noncompliant

Preconditions.checkState(a > 0, "Arg must be positive, but got " + a);  // Noncompliant. String concatenation performed even when a > 0

Preconditions.checkState(condition, formatMessage());  // Noncompliant. formatMessage() invoked regardless of condition

Preconditions.checkState(condition, "message: %s", formatMessage());  // Noncompliant
----


=== Compliant solution

[source,java]
----
logger.log(Level.SEVERE, "Something went wrong: {0} ", message);  // String formatting only applied if needed

logger.fine("An exception occurred with message: {}", message);  // SLF4J, Log4j

logger.log(Level.SEVERE, () -> "Something went wrong: " + message); // since Java 8, we can use Supplier , which will be evaluated lazily

LOG.error("Unable to open file {0}", csvPath, e);

if (LOG.isDebugEnabled()) {
  LOG.debug("Unable to open file " + csvPath, e);  // this is compliant, because it will not evaluate if log level is above debug.
}

Preconditions.checkState(arg > 0, "Arg must be positive, but got %d", a);  // String formatting only applied if needed

if (!condition) {
  throw new IllegalStateException(formatMessage());  // formatMessage() only invoked conditionally
}

if (!condition) {
  throw new IllegalStateException("message: " + formatMessage());
}
----


=== Exceptions

``++catch++`` blocks are ignored, because the performance penalty is unimportant on exceptional paths (catch block should not be a part of standard program flow). Getters are ignored as well as methods called on annotations which can be considered as getters. This rule accounts for explicit test-level testing with SLF4J methods ``++isXXXEnabled++`` and ignores the bodies of such ``++if++`` statements.


ifdef::env-github,rspecator-view[]

'''
== Implementation Specification
(visible only on this page)

=== Message

Use the built-in formatting to construct this argument.


'''
== Comments And Links
(visible only on this page)

=== on 31 Mar 2015, 15:12:32 Ann Campbell wrote:
reassigning to you [~nicolas.peru] because I've updated the formatting in the code samples & I'd like you to double-check me, please.

=== on 8 Apr 2015, 14:59:27 Nicolas Peru wrote:
seems ok.

=== on 14 Apr 2016, 10:13:49 Freddy Mallet wrote:
I would add the tag 'slf4j' [~ann.campbell.2]

=== on 14 Apr 2016, 15:41:19 Ann Campbell wrote:
I disagree [~freddy.mallet]. If this rule were specifically for and only about slf4j then I would, but this covers multiple frameworks.

=== on 17 Nov 2016, 15:41:41 Tibor Blenessy wrote:
\[~ann.campbell.2] if we want to support multiple frameworks, how should they be detected? i.e. should every string concatenation in arguments be flagged, or should the method name be used as a heuristic to find out that string concatenation will infer penalty? Other option could be to blacklist well known method signatures? If yes, what are the signatures? 

=== on 17 Nov 2016, 16:17:58 Ann Campbell wrote:
\[~tibor.blenessy] that's probably a discussion that's better to have with [~nicolas.peru] or [~michael.gumowski].

endif::env-github,rspecator-view[]