5
\$\begingroup\$

I am currently working on a new version of my application, and I just finished rebuilding the log part. It works well but I am skeptical about the choices I made. I am a student and I work alone in some side projects, and I am afraid of developing bad habits.

I use java.util.logging, because it is simple enough for what I need to do.

The logger will be used by several part of the application (but not at the same time). Each executable will have a logging file. All configurations are stored in Settings class.

Main class :

public class MyLogger
{
    public static Logger   LOGGER  = null; // Logger use by Spider, Cleaner, Checker
    private static Handler logFile = null; // Logger file

    /**
     * Initialize logger before first use
     * 
     * @param folderName sub-folder after the log folder
     * @param fileName log file name (without extension)
     */
    public static void initLogger(String folderName, String fileName)
    {
        if (MyLogger.LOGGER != null)
            throw new IllegalStateException("Logger already instantiated");

        MyLogger.LOGGER = Logger.getLogger(MyLogger.class.getName());

        // Check if log is not disable
        if (Settings.getProp().getLogLevel() != Level.OFF)
        {
            String folderPath = Settings.getProp().getLogPath() + folderName;

            File dir = new File(folderPath);

            // if the directory does not exist, create it (recursively)
            if (!dir.exists())
            {
                try
                {
                    dir.mkdirs();
                }
                catch (SecurityException e)
                {
                    e.printStackTrace(); // Throw RunTime instead ?
                }
            }

            try
            {
                // Open log file
                MyLogger.logFile = new FileHandler(folderPath + File.separator + fileName + ".log", true);
                MyLogger.logFile.setEncoding("UTF-8");
                // Attache file to logger
                MyLogger.LOGGER.addHandler(MyLogger.logFile);
            }
            catch (SecurityException | IOException e)
            {
                e.printStackTrace(); // Throw RunTime instead ?
            }
        }

        LOGGER.setLevel(Settings.getProp().getLogLevel());

        // Console Text formatter
        LOGGER.setUseParentHandlers(false);

        MyConsoleHandler handler = new MyConsoleHandler();
        handler.setLevel(Settings.getProp().getConsoleLevel());
        handler.setFormatter(new MyFormatter());
        LOGGER.addHandler(handler);

        LOGGER.config("Logger Started : " + folderName + " - " + fileName);
        LOGGER.config(Settings.getProp().toString()); // Print application settings
    }
}

Formatter :

public class MyFormatter extends Formatter
{
    // Create a DateFormat to format the logger timestamp.
    private static final DateFormat df = new SimpleDateFormat("H:mm:ss.SSS");

    /* (non-Javadoc)
     * @see java.util.logging.Formatter#format(java.util.logging.LogRecord)
     */
    @Override
    public String format(LogRecord record)
    {
        StringBuilder builder = new StringBuilder(1000);

        builder.append(df.format(new Date(record.getMillis()))).append(" "); // time
        builder.append("(").append(record.getThreadID()).append(") "); // Thread ID
        builder.append("[").append(record.getLevel()).append("] "); // level
        builder.append(formatMessage(record)); // message
        builder.append("\n");

        return builder.toString();
    }
}

Console Handler :

public class MyConsoleHandler extends ConsoleHandler
{
    /* (non-Javadoc)
     * @see java.util.logging.Handler#publish(java.util.logging.LogRecord)
     */
    @Override
    public void publish(LogRecord record)
    {
        try
        {
            if(record.getLevel().intValue() >= this.getLevel().intValue())
            {
                String message = getFormatter().format(record);
                if (record.getLevel().intValue() >= Level.WARNING.intValue())
                {
                    System.err.write(message.getBytes());                       
                }
                else
                {
                    System.out.write(message.getBytes());
                }
            }
        } catch (Exception exception)
        {
            reportError(null, exception, ErrorManager.FORMAT_FAILURE);
            return;
        }
    }
}

Example of use :

public static void main(String[] args)
{
    MyLogger.initLogger("test", "001");

    MyLogger.LOGGER.info("Testing message");
    MyLogger.LOGGER.warning("A warning message !");
}

Example of output (console) :

22:43:26.918 (1) [CONFIG] Logger Started : test - 001
22:43:26.921 (1) [CONFIG] Appication Settings : 
     - [...]
     - Console Level : CONFIG
     - Log Level : FINER
     - Log Path : ./log/
     - [...]
     - OutputQueueLimit : 1000 items
22:43:26.922 (1) [INFO] Testing message
22:43:26.922 (1) [WARNING] A warning message !
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis Aug 16 '16 at 16:33
2
\$\begingroup\$

1) Braces style
Putting opening curly braces on new lines is fine, though the big majority of Java developers don't do it and it wastes space.
Not putting curly braces around single line statements is dangerous and many people will discourage you from doing it!: https://stackoverflow.com/questions/8020228/is-it-ok-if-i-omit-curly-braces-in-java
https://softwareengineering.stackexchange.com/questions/16528/single-statement-if-block-braces-or-no/16530

2) capitalized field names
Capitalized field names (LOGGER) are classically reserved for final constants, your LOGGER is not final. Meanwhile your DateFormat df should be capitalized.

3) Properly build paths
Use the File constructor to combine path parts:
https://stackoverflow.com/questions/412380/combine-paths-in-java
I guess making your ultimate File like File file = new File(folderName, fileName + ".log") and then calling file.getParentFile().mkdirs(); would be the most pleasant invocation.
Pass the path of the file to your FileHandler constructor.

4) Throw RuntimeException on SecurityException
Your logger won't work. Something is wrong and your program shouldn't just silently ignore it! I'd prefer it to crash at that point: "Fix the logging or I won't work!"

5) SimpleDateFormat is not threadsafe!
Evil JRE developers want to secretly and unexpectedly mess up your date formatting: https://stackoverflow.com/questions/6840803/simpledateformat-thread-safety
You should at least be aware of that if you plan to use multithreading.

6) Append cascade
Your append cascade is somewhat painful to read: It's hard for me to see exactly what you are combining.
I'd put every .append() on a new line.

7) Append cascade
Is there any advantage to calling write(message.getBytes()); instead of println/print (message)? I have never seen that before.

8) Seperate loggers for different classes, no direct access
Normally you create one logger for each class it's used in and put it in a static final field. That way it's easier to control logging and find out where the log entry comes from.

9) HH instead of H
You might want to use HH instead of H in your SimpleDateFormat, so your log entries are better aligned and more consistent.

\$\endgroup\$
  • \$\begingroup\$ Great answer, can you tell me more about the point 8 ? For each execution of my application I want only one log file. \$\endgroup\$ – Opsse Aug 18 '16 at 17:15
  • \$\begingroup\$ Take a deeper look at Javas default Logging API and usage: vogella.com/tutorials/Logging/article.html You see that static final Logger instances are acquired via Logger.getLogger(CurrentClass.class.getName());. You should probably attach your custom handlers to the root logger. \$\endgroup\$ – Traubenfuchs Aug 19 '16 at 10:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.