knowm / XChart

XChart is a light-weight Java library for plotting data.
http://knowm.org/open-source/xchart
Apache License 2.0
1.48k stars 394 forks source link

Employing a logging framework? #574

Open mccartney opened 3 years ago

mccartney commented 3 years ago

Currently the code is full of System.out.println statements. Some/most of them being commented out, some of them not.

The industry standard practice is to employ a logging framework (log4j, commons-logging, java.util.logging) instead, so that:

Currently using the library makes derived programs print out something to stdout and people have no control of that.

mccartney commented 3 years ago

@timmolter what's your view on this?

timmolter commented 3 years ago

I leaned against it in the beginning as a design decision. There shouldn't be any souts that aren't commented out except for debugging and testing. I'd rather not not have to add the extra dependency. What really is of interest in a charting lib to be logged?

mccartney commented 3 years ago

As of now, in xchart/src/main there are:

What really is of interest in a charting lib to be logged? I see one area - i.e. for people troubleshooting or contributing to the library when they try to understand how the library behaves.

I see several options:

  1. Leave it like it is.
  2. Remove all the System.out.println statements, commented or not
  3. Change the System.out.println statements to usages java.util.logging.Logger, most likely calls to its .fine() or similar
  4. Introduce an in-house mini framework for logging - i.e. a singleton DebugOutput similar to:
    class DebugOutput {
    private static final boolean PRINT = false;
    public static void print(String message) {
     if (PRINT) System.out.println(message);
    }
    }
  5. Adapt log4j or another proper log-management framework.

As to my comments:

wborn commented 2 years ago

Adapt log4j or another proper log-management framework.

Rather use a logging facade like SLF4J which would allow everyone to use their favorite logging framework. It is very common to use SLF4J nowadays and only adds one extra dependency (slf4j-api, ~42kB) that has no other dependencies. Even that dependency could be made optional by wrapping the calls, catching ClassNotFoundException when it occurs and then ignore further log calls... or use the System.out.println method if some System.getProperty variable is set.

mccartney commented 1 year ago

Here's some code with System.out.printlns: https://github.com/knowm/XChart/blob/1f31cce168b26adbec479eb4f38d7c6cd9724a53/xchart/src/main/java/org/knowm/xchart/internal/chartpart/AxisTickCalculator_.java#L302-L311

mccartney commented 1 year ago

slf4j(-api) is the industry standard these days

timmolter commented 1 year ago

and System.out.printlns should be commented out before a release. slf4j would be the way to go for sure if we needed a logging framework for this project. But do we really? I have slf4j in all my other OSS projects but it doesn't make sense really to me for this one. The commented out System.out.printlns are for debugging/developing the code, at least the ones I wrote.