adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
730 stars 736 forks source link

Don't use SLF4J logger together with String.format #40

Closed kwin closed 7 years ago

kwin commented 7 years ago

Since SLF4J comes with support for parameterized messages (https://www.slf4j.org/apidocs/index.html). That should be used rather than String.format(...). Some wrong usages of String.format(...) can be seen in https://github.com/Adobe-Marketing-Cloud/aem-core-wcm-components/blob/master/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L203, https://github.com/Adobe-Marketing-Cloud/aem-core-wcm-components/blob/master/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L209, https://github.com/Adobe-Marketing-Cloud/aem-core-wcm-components/blob/master/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L418 and https://github.com/Adobe-Marketing-Cloud/aem-core-wcm-components/blob/4de23b51987cedf509f3b7e161756f6edcf9f23e/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/models/impl/v1/ImageImpl.java#L245.

raducotescu commented 7 years ago

The only potentially redundant usages are where messages are logged without a Throwable - https://github.com/Adobe-Marketing-Cloud/aem-core-wcm-components/blob/master/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L203, https://github.com/Adobe-Marketing-Cloud/aem-core-wcm-components/blob/master/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L209

kwin commented 7 years ago

IMHO the method https://www.slf4j.org/api/org/slf4j/Logger.html#debug(java.lang.String,%20java.lang.Object...) could be used even if there is a throwable given as last argument. As long as it is not referenced within the message this should be the same as using https://www.slf4j.org/api/org/slf4j/Logger.html#debug(org.slf4j.Marker,%20java.lang.String,%20java.lang.Throwable). This is also explicitly described in https://www.slf4j.org/faq.html#paramException.