Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So I've been kind of irritated with my logging recently as I find myself copying and pasting the same kinds of generic messages around to lots of different methods or retyping them ever so slightly differently (when they ought to be identical).

Hence, I created this kind of framework:

public class Log {

// other logging utility methods truncated

    public enum Message {

        //Main
        PROGRAM_EXIT ("Exiting program, return code = {0}"),
        THREAD_INTERRUPTED ("Thread interrupted while {0}"),
        FATAL_TERMINATING_ERROR ("Fatal exception thrown through entire stack: {0}"),
        SHUTDOWN_HOOK_EXCEPTION ("Exception while attempting to gracefully exit: {0}"),
        IO_READ_ATTEMPT ("Attempting to read file: {0}"),
        IO_READ_FAILURE ("Unable to read from file: {0}"),
        IO_WRITE_ATTEMPT ("Attempting to write to file: {0}"),
        IO_WRITE_FAILURE ("Unable to write to file: {0}"),
        IOEXCEPTION ("IOException while {0} to {1} : {2}"),

        private final String text;

        private Message(String text) {
            this.text = text;
        }

            @Override
            public String toString() {
                return text;
            }

        public String format(Object ... args) {
            String returnValue = toString();
            for(int i = 0; i < args.length; i++) {
                String placeholder = "{" + i + "}";
                if(!returnValue.contains(placeholder)) {
                    LOGGER.error("Too many arguments supplied to generateMessage() from " + getCallingMethodName());
                    LOGGER.error(" -- " + name());
                    break;
                }
                returnValue = returnValue.replace("{" + i + "}", args[i] == null ? "null" : args[i].toString());
            }
            return returnValue; 
        }
    }
}

It's then invoked like the following:

private static final String FILE_PATH = "resources/something.txt";
//...
LOGGER.error(Log.Message.IO_READ_FAILURE.format(FILE_PATH));

... which would log ...

Unable to read from file: resources/something.txt

It all works fine, but it feels a little too verbose at times for what I'm trying to accomplish, like I've tacked on a bit too many nuts and bolts. Like I've gone deer hunting with a .50 cal or something.

Any suggestions to improve this? The same messages are used redundantly in several places, so I am seeing some value out of it, but I'm not sure if there's a good way to make this still more clean or otherwise accomplish what I'm trying to do in a more succinct way.

If it's relevant, I'm using log4j.

share|improve this question
add comment

3 Answers

up vote 3 down vote accepted

You don't need to re-implement MessageFormat. It is much more powerful than what your implementation provides.


You don't show us the class of LOGGER, but based on the fact that you have a Log class, I suspect you are not using a logging library. I highly recommend you look into using one instead of implementing your on custom implementation.

If you are using a logging library, you are abstracting to much away from the library. All of the frameworks I've used support message formating and detailed exception logging (with stack traces).


The problem you are really trying to solve here is to allow yourself to reuse a number of common log message formats. This can be done with a simple set of Strings set to public static final.

share|improve this answer
    
LOGGER is org.apache.log4j.Logger –  Jeff Gohlke Feb 3 at 15:33
    
I actually didn't know MessageFormat was a thing in Java. I was emulating a pattern I saw in C#. Haha. Nice. –  Jeff Gohlke Feb 3 at 15:40
    
@JeffGohlke: A quick search shows that log4j 1.x does not support message formating. log4j 2.x will be out of beta soon and does support it. Another option is to use slf4j for a consistent logging API, that supports message formating, and will let you migrate from log4j 1.x to 2.x, or a different framework, without any changes to your code. –  unholysampler Feb 3 at 15:44
add comment

Well,

It does what it is supposed to and works fine. My comment is more regarding the logger class than the enum of messages.

You indicated you use this logger throughout your project, well if you change your messages add/remove some you would have to recompile a lot of stuff.

Personally I would have an interface for logger and instead of reading strings read in an exception.

you could then choose to make some of the repetitious exceptions reusable in a similar fashion to above.

This would decouple everything.

The advantage would be that if one message changed only that file would, if you wanted to add an exception...just throw a different exception and finally if you wanted to change your logging means you just have to change your implementation.

interface Logger
{
   Log(Exception ex);
}

class ConsoleLogger implements Logger { ... }

public class Main
{
  Logger logger = new ConsoleLogger();


  public static void main(string[] args)
  {
     try
     {
       ...
     }catch(Exception ex)
    { 
      logger.Log(ex);
    }

  }

}

Of course if your logger is not only logging exceptions but messages also this may require a reworking to support maybe a custom LogMessage interface or something and it could either be a message or a exception...

but that could very well be too verbose. in either case regardless of how you send your messages the point I wanted to highlight was for something as transmutable as a Logger with static types,

a more malleable option will save you headaches in the long run...

share|improve this answer
add comment

Using the same log message too many times could indicate that the code has some duplication. If that's the case you might want to solve this instead of curing the symptoms. For example, you could write a file reader method/class, modify the project to read files using only this class. (There are existing libraries for that.) Only this class should log messages like IO_READ_ATTEMPT and IO_READ_FAILURE (or throw exceptions with these messages) because every IO read errors happens in it. (It would improve the abstraction level of the application too. For example, you could easily switch to another file storage from file system since every storage logic would be in the same place.)


If you don't know SLF4J, check its parametrized messages. For example, in SLF4J,

logger.debug("The entry is {}.", entry);

calls entry.toString() only, when debug logging is enabled. Otherwise, it does not do any string concatenation/processing.


I would consider another approach too: wrap the Logger instance, for example:

public class FileOperationsLogger {

    private final Logger logger;

    public FileOperationsLogger(Logger logger) {
        this.logger = checkNotNull(logger, "logger cannot be null");
    }

    // delegate error/warn/info/debug methods to the logger instance

    public void logIoReadFailure(final String filename) {
        logger.error("Unable to read from file: {}", filename);
    }

    // ... other specific log methods with messages
}

It would provide type safety (filename could be only String).

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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