google / styleguide

Style guides for Google-originated open-source projects
https://google.github.io/styleguide/
Apache License 2.0
37.23k stars 13.3k forks source link

Java Style Guide: Constants example (Joiner vs Logger) #303

Open vgonzalo opened 6 years ago

vgonzalo commented 6 years ago

I've seen many discussions about if a static final Logger instance should be considered a constant or not (i.e. uppercase letters), and there are references to this style guide in which is suggested to use the logger as a non-constant value:

static final Logger logger = Logger.getLogger(MyClass.getName());

However, in most of the cases (at least all that I know), the Logger instances are immutables because there are no methods to change the internal status or to retrieve a mutable object, which seems to be the same case of the Joiner:

static final Joiner COMMA_JOINER = Joiner.on(','); // because Joiner is immutable

What is the fundamental difference between a Logger and a Joiner to differentiate them as a constant or non-constant?

What would be the case of a Gson instance if declared as a static final?

Thanks in advance

danielburrell commented 6 years ago

Where in the style guide did you see COMMA_JOINER ?

vgonzalo commented 6 years ago

https://github.com/google/styleguide/blob/gh-pages/javaguide.html#L874

facboy commented 6 years ago

Constants are static final fields whose contents are deeply immutable and whose methods have no detectable side effects.

This is the difference, no?

vgonzalo commented 6 years ago

Probably, but it is still a weak rule to do this determination, e.g. what if the Joiner given as an example has a caching mechanism internally? What if you are specifying an interface but you have implementations with and without side effects? How do you know that a Logger has detectable side effects? Do we need to read and understand all of our dependencies in order to know if they can be used as a constant or not? Due to these questions I asked what would be the case of a Gson static final instance.

Delfic commented 6 years ago

@vgonzalo They do have this on their documentation: "Warning: joiner instances are always immutable; a configuration method such as useForNull has no effect on the instance it is invoked on! You must store and use the new joiner instance returned by the method. This makes joiners thread-safe, and safe to store as static final constants."

Whilst in a Logger you have state such as logging levels and registered handlers. Using methods such as addHandler and setLevel change the state so it's not a constant in the same sense as truly Immutable instances.

Gson has type adapters that can be registered so I would argue that they are not constants.

vgonzalo commented 6 years ago

@Delfic I think you are enforcing my point: requiring to read the documentation about its implementation or understanding the internal algorithms is a weak styling rule because implementations can change over time (e.g. as I mentioned above, adding an internal caching mechanism can break this definition of immutability even if it's thread safe).

About the Logger, those mutators are present in the java.util.logging.Logger only, however in log4j2 or slf4j, for example, the LogManager gives you a Logger instance without mutators.

Delfic commented 6 years ago

@vgonzalo Styling rules serve two main purposes: Better the readability of the code and prevent VCS conflicts coming from different styles. If you go with one choice for all log implementations you prevent the latter but if you do care about the internal implementations and abide by it you address the former. That being said it's a rule that seems impossible to enforce by static analysis.