Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration to SLF4J #22

Merged
merged 2 commits into from
Apr 3, 2023
Merged

Migration to SLF4J #22

merged 2 commits into from
Apr 3, 2023

Conversation

BBE78
Copy link
Contributor

@BBE78 BBE78 commented Mar 23, 2023

Use of SLF4J logger instead of System.(out|err).println()

Use of SLF4J logger instead of System.(out|err).println()
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask you to remove all if (LOGGER.isDebugEnabled()) wrappers since it is not necessary anymore with the logging framework in place...

@BBE78
Copy link
Contributor Author

BBE78 commented Mar 30, 2023

I ask you to remove all if (LOGGER.isDebugEnabled()) wrappers since it is not necessary anymore with the logging framework in place...

Hi @danielpeintner, checking the log level before logging is a "best practice" to avoid memory allocation, string concatenation, for nothing... This rule is raised by several tools such as SonarQube https://rules.sonarsource.com/java/RSPEC-2629
I'm used to apply this recommendation in my source code, do you still want the check to be removed?

@danielpeintner
Copy link
Member

danielpeintner commented Mar 31, 2023

I don't have a very strong opinion but I guess we both can agree that the code looks nicer and less messy without this is additional if (LOGGER.isDebugEnabled()) { wrapper...

Moreover, with the formatting style of
LOGGER.debug("Foo '{}' is happening", "x");
the string is actually not concatenated.

It would be different if we do something like the following
LOGGER.debug("Foo " + "xyz" + " is happening);

see also some information here: https://logging.apache.org/log4j/log4j-2.3.2/performance.html

Anyhow, as I said, IF YOU STRONGLY PREFER to keep it as is I will not argue strongly... :)

@BBE78
Copy link
Contributor Author

BBE78 commented Mar 31, 2023

All debug level checks removed

@danielpeintner danielpeintner merged commit 9ce14e6 into EXIficient:master Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants