Code sample: logger.debug("Today is {}", DayOfWeek.MONDAY.name());
When logging enums, it’s often clearer, simpler and more stable to log the enum name rather than the whole state of the enum (which may be rendered by toString). This generally produces clearer log messages and uses less disk space. However, doing this triggers rule S2629, a major code smell, indicating that logging arguments should not require evaluation.
In the specific case of an enum name, this is a false positive. The enum name is stored in the implicit parent class Enum of an enumeration. There, name() is a final method there referring to the name variable. By the time a line such as the example is executed, the name string will necessarily live in the VM’s permgen. Thus, the invokation of enum.name() is trivial.
Enum.name() should be considered as an exception to rule S2629.
Most programmers should use the toString() method in preference to this one, as the toString method may return a more user-friendly name. This method is designed primarily for use in specialized situations where correctness depends on getting the exact name, which will not vary from release to release.
So I would not recommend relying on name() extensively.
Thanks Tibor - and thanks for your quick follow-up! As you can tell by the fact that we’re following this, we really appreciate the time and thought that’s in Sonar and its ruleset.
While everything you’ve mentioned is true, it doesn’t really address the fact that raising a violation of S2629 for this case seems like a false positive.
Presumably S2629 exists to ensure that a potentially expensive string rendering for a log message that may not actually log anything is lazily evaluated. The suggestions to avoid this are:
to use the logger’s templating mechanism so that toString is lazily invoked (only works if toString is the appropriate method)
to guard the logger call in an if(logger.isLevelEnabled()) block
But in this case, solution 1 won’t work, and solution 2 is more computationally expensive that simply inserting the enum name.
Do you agree that a call to Enum.name() is a false positive trigger for S2629?
/**
* Returns the name of this enum constant, as contained in the
* declaration. This method may be overridden, though it typically
* isn't necessary or desirable. An enum type should override this
* method when a more "programmer-friendly" string form exists.
*
* @return the name of this enum constant
*/
public String toString() {
return name;
}
If you use java.time.DateOfWeek (see frohoff/jdk8u-dev-jdk) then this class doesn’t overwrite toString.
For me it sounds that you can use:
logger.debug("Today is {}", DayOfWeek.MONDAY);
because it works exactly the same.
I wouldn’t recommended changing code to fix static analysis Won’t Fix or Falser Positives issues. I think that:
String mondayDayName= DayOfWeek.MONDAY.name()
logger.debug("Today is {}", mondayDayName);
only make code longer and it is even less readable than simple:
logger.debug("Today is {}", DayOfWeek.MONDAY.name());