'SonarQube: Invoke method(s) only conditionally

The following part of code raises a major bug at SonarQube : "Invoke method(s) only conditionally."
How am I supposed to fix this?

if(us != null){
    logger.info("Log this: {}", us.toString());
}
javaloggingsonarqube


Solution 1:[1]

The call to us.toString() is redundant, toString() method will be called regardless the configured log level. You should pass only us as an argument to info without an if statement.

logger.info("Log this: {}", us);

Solution 2:[2]

As stated at the comments of the question, another working answer is:

if(logger.isInfoEnabled() && us != null){
    logger.info("Log this: {}", us.toString());
}

Solution 3:[3]

You can just ignore this but it might be good to handle this scenario if possible, It would help us to handle and cutoff unnecessary computations.

One thing what it suggests here is to check if the Log Level that you are going to use is enabled or not. 

if(logger.isInfoEnabled() && us != null){
    // this inner code will only get executed if the above is true
    logger.info("Log this: {}", us.toString());
}

Imagine having a complex task running inside, it would be a waste of time to do that if you are not going to log it anyways, if the log level is disabled. Logger will internally check that for you but doing it now before invoking the .info() will save you some cycles.

Solution 4:[4]

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 a 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

    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

    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.

Solution 5:[5]

Short easy answer: just delete the .toString from your code since the formatter will take care of changing it to String for you.

Solution 6:[6]

It's related to performance issues. They recomend just putting a pure String or a final variable defined previously.

final var message = "Log this: " + us.toString();
logger.info(message);

https://sonarcloud.io/organizations/default/rules?languages=java&open=java%3AS2629&q=S2629

Sources

This article follows the attribution requirements of Stack Overflow and is licensed under CC BY-SA 3.0.

Source: Stack Overflow

Solution Source
Solution 1 Olezt
Solution 2 Olezt
Solution 3 prime
Solution 4 Anurag Sharma
Solution 5 Sam
Solution 6 gleitonfranco

Related Questions