3
\$\begingroup\$

I'm currently working on a small project that uses Java to handle a socket-based MMO browser game. I'm still at the beginning of the project and I'm currently writing a class that logs errors and debug messages to the console and writes these to the appropriate file, based on the error.

I'm wondering if this code is clean, efficient, readable, and amendable in any way shape or form.

package nl.finicky.lemonade.core;
import nl.finicky.lemonade.Lemonade;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;

public class Console {
    public static final String ANSI_RESET = "\u001B[0m";
    public static final String ANSI_BRIGHT = "\u001B[1m";
    public static final String ANSI_BLACK = "\u001B[30m";
    public static final String ANSI_RED = "\u001B[31m";
    public static final String ANSI_GREEN = "\u001B[32m";
    public static final String ANSI_YELLOW = "\u001B[33m";
    public static final String ANSI_BLUE = "\u001B[34m";
    public static final String ANSI_PURPLE = "\u001B[35m";
    public static final String ANSI_CYAN = "\u001B[36m";
    public static final String ANSI_WHITE = "\u001B[37m";
    private static File debug = new File("logs//debug.txt");
    private static File packets = new File("logs//packets.txt");
    private static File errors = new File("logs//errors.txt");

    private static PrintWriter debugWriter;
    private static PrintWriter packetWriter;
    private static PrintWriter errorWriter;

    public Console() {
        try {
            if (!debug.exists()) {
                debug.createNewFile();
            }

            if (!packets.exists()) {
                packets.createNewFile();
            }

            if (!errors.exists()) {
                errors.createNewFile();
            }

            debugWriter = new PrintWriter(new FileWriter(debug));
            packetWriter = new PrintWriter(new FileWriter(packets));
            errorWriter = new PrintWriter(new FileWriter(errors));
        } catch (IOException e) {
            e.printStackTrace();
        }

    }

    public void start(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }

        System.out.println(ANSI_GREEN + ANSI_BRIGHT + "[START] " + message.toString() + ANSI_RESET);
    }
    public void user(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }

        System.out.println(ANSI_BLUE + "[USER] " + message.toString() + ANSI_RESET);
    }

    public void shutdown(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }
        System.out.println(ANSI_YELLOW + ANSI_BRIGHT + "[SHUTDOWN] " + message.toString() + ANSI_RESET);
    }

    public synchronized void error(Object message) {
        errorWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        System.out.println(ANSI_RED + "[ERROR] " + message.toString() + ANSI_RESET);
    }

    public synchronized void critical(Object message) {
        errorWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        System.out.println(ANSI_RED + "[ERROR] " + message.toString() + ANSI_RESET);
    }

    public synchronized void debug(Object message) {
        if (Lemonade.getConfiguration().getBool("logging.debug")) {
            debugWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }

        System.out.println(ANSI_PURPLE + "[DEBUG] " + message.toString() + ANSI_RESET);
    }

    public synchronized void packet(Object message)
    {
        System.out.println(ANSI_BLUE + "[PACKET] " + message.toString() + ANSI_RESET);

        if(Lemonade.getConfiguration().getBool("logging.packets"))
        {
            packetWriter.write(Lemonade.getDate() + message.toString() + "\r\n");
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Why not use log4j? \$\endgroup\$ – dwjohnston Aug 16 '16 at 0:29
2
\$\begingroup\$

A synchronized method will block execution of other methods in the class, you better use a seperate thread to store the logsentries to write.

This could be a simple write-only-thread.

.error and .critical are the same, this is violating DRY.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I can't suggest you anything about design of the program (because you developed it and you know more about it) but here are some suggestions:

1) Group constants into static inner classes. For example, convert:

public static final String ANSI_RESET = "\u001B[0m";
public static final String ANSI_BRIGHT = "\u001B[1m";
public static final String ANSI_BLACK = "\u001B[30m";
public static final String ANSI_RED = "\u001B[31m";
public static final String ANSI_GREEN = "\u001B[32m";
public static final String ANSI_YELLOW = "\u001B[33m";
public static final String ANSI_BLUE = "\u001B[34m";
public static final String ANSI_PURPLE = "\u001B[35m";
public static final String ANSI_CYAN = "\u001B[36m";
public static final String ANSI_WHITE = "\u001B[37m";

into something like this:

public static class AnsiControlCharacter {
    public static final String RESET = "\u001B[0m";
    public static final String BRIGHT = "\u001B[1m";

    public static class Color {
        public static final String BLACK = "\u001B[30m";
        public static final String RED = "\u001B[31m";
        public static final String GREEN = "\u001B[32m";
        public static final String YELLOW = "\u001B[33m";
        public static final String BLUE = "\u001B[34m";
        public static final String PURPLE = "\u001B[35m";
        public static final String CYAN = "\u001B[36m";
        public static final String WHITE = "\u001B[37m";
    }
}

So you get more clean structure (again, it's only my opinion) and IDE's auto complete makes magic too. You can group Files and PrintWriters as well.

2) Do not use "\r\n" as line ending (unless you really need to be platform-specific). Use %n in String.format() or line separator from System.getProperty("line.separator"); or use System.lineSeparator() (in Java 7) or appeal to java.util.Formatter class (in Java 8). Perhaps, the best way is to use log4j as mentioned by @dwjohnston. But if you strive to make minimalistic and totally independent application involving library can bring some pain. But anyway it seems reasonable to extract common logic (like if (Lemonade.getConfiguration().getBool("logging.debug")) { ...) into utility classes. BTW there is logging functionality in java.util.logging package, but seems that nobody uses it...

3) As for synchronization I can't tell anything substantial because I don't know how the code will be invoked (seems that you deal with some virtual or physical console...).

4) Take care of releasing PrintWriters when they are not used (again, it's just suggestion, I don't know about life cycle of your application).

PS Sorry, this stuff should be a comment, but I've posted it as answer because of text format...

| improve this answer | |
\$\endgroup\$

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.