6
\$\begingroup\$

I recently made a logger program implementation in my game engine. Which logs any thing that I send it in either a file or the System.out printstream.

I use it like this:

private static Logger logger = LogManager.getLogger(Shader.class.getName());

And then I can call a log method like this:

logger.info("Info", e); // I can also pass it an exception or a throwable for it to print to the stream or write to the file.

This is a sample output (it writes the same thing to a file as well)

[July 24, 2019 7:42 PM] [com.base.engine.core.CoreEngine] INFO: 4.5.13399 Compatibility Profile Context 15.200.1062.1004

The reason I did this was because one day I thought to myself, why not log things to a file.

So I went on my crazy 4 hour journey to reinvent the wheel and make a logger from scratch. Mostly the reason was that I couldn't figure out how to get log4j working so I gave up and did this.

My question is that: Is this in someone's opinion, efficient enough to be used in production and is there a way to make it better, maybe by multithreading it?.

Here is the LogManager:

public class LogManager {
    private static HashMap<String, Logger> loggers = new HashMap<String, Logger>();
    private static ArrayList<LogLevel> allowedLevels = new ArrayList<LogLevel>();
    private static LogLevel currentLevel;
    public static FileHandler fileHandler;
    public static Logger getLogger(String className) {
        if (!loggers.containsKey(className)) {
            Logger resultLogger = new Logger(className);
            loggers.put(className, resultLogger);
            if (fileHandler != null) {
                resultLogger.setOutputForLogFile(fileHandler.out);
            }
        }
        return (loggers.get(className));
    }
    public static void addFileHandler() {
        if (fileHandler == null) {
            try {
                fileHandler = new FileHandler("log.log", "./logs/");
                fileHandler.setAppend(false);
                fileHandler.initializeWriter();
                fileHandler.out.println("\n-------------------------------------" + getCurrentTimeAndDate()
                        + "-------------------------------------\n");
                for (Entry<String, Logger> entry : loggers.entrySet()) {
                    String key = entry.getKey();
                    Logger value = entry.getValue();

                    System.out.println("Adding output " + key);
                    value.setOutputForLogFile(fileHandler.out);
                }
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
    public static void addAllowedLevel(LogLevel l) {
        if (!allowedLevels.contains(l)) {
            allowedLevels.add(l);
        }
    }
    public static void removeAllowedLevel(LogLevel l) {
        if (!allowedLevels.contains(l)) {
            allowedLevels.remove(l);
        }
    }
    public static boolean isLevelAllowed(LogLevel l) {
        if (allowedLevels.contains(l)) {
            return (true);
        }
        return (false);
    }
    public static void setLogLevel(LogLevel l) {
        currentLevel = l;
    }
    public static HashMap<String, Logger> getLoggers() {
        return loggers;
    }
    public static void setLoggers(HashMap<String, Logger> loggers) {
        LogManager.loggers = loggers;
    }
    public static LogLevel getCurrentLevel() {
        return currentLevel;
    }
    public static void setCurrentLevel(LogLevel currentLevel) {
        LogManager.currentLevel = currentLevel;
    }
    public static String getCurrentTimeAndDate() {
        return (DateFormat.getDateTimeInstance(DateFormat.LONG, DateFormat.SHORT).format(new Date()));
    }

    static SimpleDateFormat sdf = new SimpleDateFormat("yyyyy.MMMMM.dd GGG hh:mm aaa");

    private static String getCurrentTimeAndDateForFile() {
        return (sdf.format(new Date()));
    }

    public static void setFileHandler(FileHandler fileHandler) {
        LogManager.fileHandler = fileHandler;
    }
}

Here is the Logger class:

public class Logger {
    private String name;
    private PrintWriter out;
    public Logger(String name) {
        this.name = name;
        // this.out = LogManager.fileHandler.out;
    }
    public Logger(String name, FileHandler fileHandler) {
        this.name = name;
        // this.out = LogManager.fileHandler.out;
    }
    public void setOutputForLogFile(PrintWriter out) {
        this.out = out;
    }
    public void setClassName(String className) {
        this.name = className;
    }
    private void println(String line) {
        System.out.println(line);
        if (out != null) {
            this.out.println(line);
            this.out.flush();
        }
    }
    private void println(Throwable e) {
        e.printStackTrace();
        if (out != null) {
            this.out.println(e.getClass().getSimpleName() + ": ");
            e.printStackTrace(this.out);
            this.out.flush();
        }
    }
    public void log(String msg) {
        println(this.formatLog(LogLevel.ALL) + msg);
    }
    public void log(String msg, Throwable e) {
        println(this.formatLog(LogLevel.ALL) + msg);
        println(e);
    }
    private String formatLog(LogLevel l) {
        String log = "[" + LogManager.getCurrentTimeAndDate() + "] [" + this.name + "] " + l + ": ";
        return log;
    }
    private boolean checkAllowed(LogLevel l) {
        if (LogManager.getCurrentLevel() == l || LogManager.getCurrentLevel() == LogLevel.ALL
                || (LogManager.isLevelAllowed(l) || LogManager.isLevelAllowed(LogLevel.ALL))) {
            return (true);
        }
        return (false);
    }
    public boolean finnest(String msg) {
        if (checkAllowed(LogLevel.FINNEST)) {
            println(this.formatLog(LogLevel.FINNEST) + msg);
            return (true);
        }
        return (false);
    }
    public void finnest(String msg, Throwable e) {
        if (finnest(msg)) {
            // e.printStackTrace();
            println(e);
        }
    }
    public boolean fine(String msg) {
        if (checkAllowed(LogLevel.FINE)) {
            println(this.formatLog(LogLevel.FINE) + msg);
            return (true);
        }
        return (false);
    }
    public void fine(String msg, Throwable e) {
        if (fine(msg)) {
            println(e);
        }
    }
    public boolean debug(String msg) {
        if (checkAllowed(LogLevel.DEBUG)) {
            println(this.formatLog(LogLevel.DEBUG) + msg);
            return (true);
        }
        return (false);
    }
    public void debug(String msg, Throwable e) {
        if (debug(msg)) {
            println(e);
        }
    }
    public boolean info(String msg) {
        if (checkAllowed(LogLevel.INFO)) {
            println(this.formatLog(LogLevel.INFO) + msg);
            return (true);
        }
        return (false);
    }
    public void info(String msg, Throwable e) {
        if (info(msg)) {
            println(e);
        }
    }
    public boolean warning(String msg) {
        if (checkAllowed(LogLevel.WARNING)) {
            println(this.formatLog(LogLevel.WARNING) + msg);
            return (true);
        }
        return (false);
    }
    public void warning(String msg, Throwable e) {
        if (warning(msg)) {
            println(e);
        }
    }
    public boolean error(String msg) {
        if (checkAllowed(LogLevel.ERROR)) {
            println(this.formatLog(LogLevel.ERROR) + msg);
            return (true);
        }
        return (false);
    }
    public void error(String msg, Throwable e) {
        if (error(msg)) {
            println(e);
        }
    }
}

And finally here is the FileHandler class:

public class FileHandler {
    private static Logger logger = LogManager.getLogger(FileHandler.class.getName());
    private String fileName, filePath;
    private File file;
    public PrintWriter out;
    private BufferedReader bufferedReader;
    private boolean append, autoFlush = true;
    /**
     * 
     * @param fileName Needs to have the name of the file with the file extension
     *                 included
     * @param filePath The complete path to the file or relative ./
     * @throws IOException
     */
    public FileHandler(String fileName, String filePath) throws IOException {
        this.fileName = fileName;
        this.filePath = filePath;
        this.file = new File(filePath + fileName);
        if (!checkFileExists()) {
            file.createNewFile();
        }
        this.append = false;
    }
    /**
     * setAppend Needs To Be Called Before Initializing Writer If The File Already
     * Exists. Otherwise It Will Overwrite The End Of The File.
     * 
     * @throws IOException
     */
    public void initializeWriter() throws IOException {
        if (!checkFileExists()) {
            logger.debug("File Dose Not Exist Creating: " + filePath + fileName);
            file.createNewFile();
        }
        out = new PrintWriter(new FileOutputStream(file, append));
    }
    public void initializeReader() throws IOException {
        File file = new File(filePath + fileName);

        bufferedReader = new BufferedReader(new FileReader(file));

    }
    public boolean checkFileExists() {
        return file.exists();
    }
    public void write(String line) throws IOException {

        if (out == null) {
            new Exception("Buffered Writer Not Initialized.").printStackTrace();
            return;
        }
        System.out.println("ss");
        out.println(line);
    }
    public String readLine() throws IOException {
        if (bufferedReader == null) {
            new Exception("Buffered Writer Not Initialized.").printStackTrace();
            return null;
        }
        return bufferedReader.readLine();
    }
    public void destroy() {
        this.finalize();
    }
    @Override
    public void finalize() {
        try {
            super.finalize();
            out.close();
            bufferedReader.close();
        } catch (Throwable e) {
            logger.error("Unable to finalize file handler.", e);
            // e.printStackTrace();
        }
    }
    public static Logger getLogger() {
        return logger;
    }
    public static void setLogger(Logger logger) {
        FileHandler.logger = logger;
    }
    public String getFileName() {
        return fileName;
    }
    public void setFileName(String fileName) {
        this.fileName = fileName;
    }
    public String getFilePath() {
        return filePath;
    }
    public void setFilePath(String filePath) {
        this.filePath = filePath;
    }
    public File getFile() {
        return file;
    }
    public void setFile(File file) {
        this.file = file;
    }
    public BufferedReader getBufferedReader() {
        return bufferedReader;
    }
    public void setBufferedReader(BufferedReader bufferedReader) {
        this.bufferedReader = bufferedReader;
    }
    public PrintWriter getOut() {
        return out;
    }
    public void setOut(PrintWriter out) {
        this.out = out;
    }
    public void setAppend(boolean append) {
        this.append = append;
    }
    /**
     * @return the autoFlush
     */
    public boolean isAutoFlush() {
        return autoFlush;
    }
    /**
     * @param autoFlush the autoFlush to set
     */
    public void autoFlush() {
        this.autoFlush = true;
    }
}

Here is a link to the full package on my github page: github

\$\endgroup\$
6
  • 2
    \$\begingroup\$ For common aspects, such as logging, I would never re-invent the wheel. There are mature frameworks available that are not at end of lifecycle and provide lots of customization and extension points. \$\endgroup\$
    – dfhwze
    Jul 25, 2019 at 15:46
  • \$\begingroup\$ yes of course but is this a good attempt at this, I know that I can flush the print writer all the way at the end of the program but how would I do so if terminates or crashed without the proper call to the stop method \$\endgroup\$
    – Boozie
    Jul 25, 2019 at 15:48
  • 1
    \$\begingroup\$ Ok if you want to use a custom logger, perhaps you should tag your question with 'reinventing-the-wheel' to avoid comments like mine :) \$\endgroup\$
    – dfhwze
    Jul 25, 2019 at 15:49
  • 1
    \$\begingroup\$ oh thanks I'm pretty new to stack exchange so didn't know you could have a tag like that. \$\endgroup\$
    – Boozie
    Jul 25, 2019 at 15:51
  • 1
    \$\begingroup\$ @JosephWood it's actually a problem because you're writing to the same file. Without concurrency control, log entries could be interleaved. You generally want to avoid that. In addition most OSes will bundle writes by committing to RAM and then writing larger chunks at once. This may or may not alleviate log entry interleaving. If your bottleneck is logging, you're logging too much anyways... \$\endgroup\$
    – Vogel612
    Jul 28, 2019 at 0:37

2 Answers 2

2
\$\begingroup\$

Usage

Determining whether to log for a given level requires a comparison level >= minimumConfiguredLevel (pseudo code) rather than an exact match or check against a set of preconfigured levels.

Thread-Safety

None of the methods are thread-safe. This could impact the state of LogManager, Logger and the file you are writing to.

Consistency

You provide 2 methods that do exactly the same.

public static void setLogLevel(LogLevel l) {
    currentLevel = l;
}

public static void setCurrentLevel(LogLevel currentLevel) {
    LogManager.currentLevel = currentLevel;
}

API Integrity

You can set a level that is not allowed.

public static boolean isLevelAllowed(LogLevel l) {
    if (allowedLevels.contains(l)) {
        return (true);
    }
    return (false);
}

public static void setLogLevel(LogLevel l) {
    currentLevel = l;
}

In addition:

  • exceptions during logging that aren't IOException make your calling code having to deal with them
  • the calling thread is impacted by the logging (flushing to file all the time).
\$\endgroup\$
5
  • \$\begingroup\$ Thanks for responding, i still don't understand what you mean by thread safety i am still a beginner so i don't know that much. And About the flush every write i have fixed that and it is now happens when the program exits. But how would I do this if it crashes or I terminate it without calling the proper stop method \$\endgroup\$
    – Boozie
    Jul 25, 2019 at 17:11
  • \$\begingroup\$ your methods are not synchronized: read about this here: stackoverflow.com/questions/21812396/… \$\endgroup\$
    – dfhwze
    Jul 25, 2019 at 17:13
  • \$\begingroup\$ but doesn't synchronizing a method only let it be called once and not multiple times? \$\endgroup\$
    – Boozie
    Jul 25, 2019 at 17:57
  • \$\begingroup\$ One at a time, if multiple threads race to get access to it: baeldung.com/java-synchronized \$\endgroup\$
    – dfhwze
    Jul 25, 2019 at 17:58
  • \$\begingroup\$ That's way to complicated for what I was trying to achieve, I honestly just wanted a simple logging program that could log what I sent it. Anyways the thread safety doesn't really matter in my application anyways so ill take the rest of your advice thanks. \$\endgroup\$
    – Boozie
    Jul 25, 2019 at 18:28
4
\$\begingroup\$

Implementation

Simple fixes

The following section contains a handful of simple fixes that I won't really justify much, see them as quickfire opinions:

  • Make the type of a field (or variable) declaration as high up the hierarchy as possible. This means HashMap should be Map, ArrayList should be List and so on.
  • Use the diamond operator for initializations with generics. new HashMap<String, Logger>() turns into new HashMap<>() that way. This is not always available.
  • Have an empty line between method bodies.
  • Use guard clauses to reduce the level of nesting with early returns. e.g. in addFileHandler start with:
    if (fileHandler != null) { 
        return;
    }
    
    • The same advice as for declaration types applies for method return types. You generally don't want to have a specific type returned if you can use an interface.
    • Do not put parentheses around return values. They are not performing an actual function in that manner.
    • finnest should be spelled finest in all instances.
    • A filed should be declared on it's own line every time.

Not so simple fixes

This section goes a bit more into depth. The changes suggested here require a more intimate familiarity with the Java libraries. A lot of the suggestions can carry over to other languages as well, though.

getLogger should make use of computeIfAbsent, which drastically simplifies the code to:

return loggers.computeIfAbsent(className, name -> {
    Logger result = new Logger(name);
    if (fileHandler != null) { 
        result.setOutputForLogFile(fileHandler.out);
    }
    return result;
});

Notice that we don't need to check for the presence of the key (containsKey) and we don't need to manually put the result into the map either.

Carrying on to addFileHandler I personally would add an additional empty line before the for loop, just to separate it from the rest.

Moving on to the allowedLevels operations.
The semantics you are looking for here (no duplicates, quick contains check) are best encapsulated in a Set. Changing allowedLevels to be a Set<LogLevel> would vastly simplify your code (and speed it up) since you don't need to perform contains-checks (which are linear in an ArrayList) to avoid duplicate items. The implementations simplify to:

addAllowedLevel(LogLevel l) { allowedLevels.add(l); }
removeAllowedLevel(LogLevel l) { allowedLevels.remove(l); }
isLevelAllowed(LogLevel l) { return allowedLevels.contains(l); }

Now we can take a look at Logger.

The first thing I noticed is that your Logger can change output. Generally you wouldn't want to expose that kind of capability to the user, especially not in a multithreaded environment. Results can be somewhat unpredictable.

Next I noticed that log does not perform any kind of checking whether logging is enabled at all. If no log-level were allowed, log should not be writing any output. Unfortunately it does...

checkAllowed can be simplified to directly return the condition you specified. Note also that the check you implemented there does not account for the typical use-case of hierarchical log-levels. I'd expect WARN and ERROR messages to be logged when I allow the INFO level. In your implementation I'd explicitly need to specify that all the log levels of INFO and higher priority are allowed.

Additionally you're not accounting for the idea of different loggers being configured differently wrt. the logging level they have.

Sidenote: you're abusing a boolean return of the overloads without Throwable to implement the overloads with throwable. This is somewhat ugly because you're exposing internal behaviours to external consumers.

also: formatLog should be implemented in terms of String.format like so:

    return String.format("[%s] [%s] %s: ", LogManager.getCurrentTimeAndDate(), name, l);

Lastly a quick look at FileHandler before comparing this to a logging library like Log4J.

You're not actually using autoFlush, it doesn't change any behaviour whether it's enabled or not. Remove unused fields.

In addition your code should be using the java.nio.file API instead of the java.io.file API (which is predating the nio API by almost a decade).

fileName, filePath and file could (and should) be declared as final. They are not expected to ever change. In fact, fileName and filePath are only used in the constructor and shouldn't be fields at all.

It's most likely also a bad idea to have a Reader and a Writer open on the same file. Results from concurrent operations and non-flushed operations are very likely to be unpredictable and / or dangerous.

Exposing getters and setters for the writer, reader and file is a bad idea as well. It allows external consumer to change the internal state of your class in ways that you don't want to have.

Comparison to existing logging libraries

Let me put it like this: You're not coming away in a good shape from this comparison. A large number of almost necessary features is completely missing from your implementation. Here some examples:

  • Log-Level definitions depending on the package
  • Log-Format customizations
  • Hierarchical Log-Level behaviour
  • Multi-Backend logging (one logger logging to multiple places)
  • Log-Message formatting with arbitrary parameters
  • Basic Threadsafety
\$\endgroup\$
2
  • \$\begingroup\$ I was about to answer, when I saw '1 new answer'. There isn't much I could add to this one :p This will make my answer considerably shorter \$\endgroup\$
    – dfhwze
    Jul 25, 2019 at 16:35
  • \$\begingroup\$ Most of the things you said were right and i had fixed most of these last night before this question was migrated here, and i was afk for a while so I didn't have time to properly edit the post. Thanks for the response. I will defiantly take your advice , it was very in depth. About the Map and List changes I heard that Hash map was faster Than map. And about the compute if absent that completely got out of my mind while i was doing this thanks. \$\endgroup\$
    – Boozie
    Jul 25, 2019 at 17:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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