I found a few minutes to review the code this evening. There are several things I don't care for in this implementation: 1. While doable, plugging in new resolver implementations is kind of a pain. You have to create an I18NLogManager, create the custom resolvers and presumably new annotation resolvers, add them to the CompositeResolvers then add those to the manager. Finally a new I18nLoggerFactory has to be created taking the manager and setINSTANCE called. 2. The resolvers provide no mechanism for reloading the bundles if they are modified. Many environments want 24x7 up time and do not want to restart servers just to change message text. 3. The implementation only supports a single Locale - the default. 4. I dislike immensely having the log level in one property file and the message text in another. This is very error prone. If you want log levels then get rid of the property files and switch to using XML where the log level and message can be bound together. Of course, this runs the risk that the level might be different from one language to another. Another option - which makes far more sense to me - is to not even have a LogLevelResolver, just require the log level be defined in the enum and just use that. 5. Messages are always resolved in the writeLog method. This precludes the option of the message keys being written to a database so the message can be retrieved based on the user's locale or passing the message to different machines where it can be logged using a Locale appropriate for that environment. 6. If you really, really want to use ResourceBundle's then at least make Java 6 the minimum version and provide support for ResourceBundle.Control. However, since ResourceBundles don't provide support for reloading I'd avoid using them at all. 7. It seems very strange to have the LogLevel enum have log methods and that writeLog is actually calling them. 8. The callerData is going to be useless since the LogLevel enum is just using the standard SLF4J apis. Instead, the Logger needs to extend org.slf4j.ext.LoggerWrapper and actually log from that class. 9. The format of the LogMessages enum is:

public enum LogMessages {

@Error("error message {}") // The log level is bound to Error. TEST0001,

@Message("waring message {}") // The log level is not specified. TEST0002

}

a. why would you want to allow a log level to not be specified? (See item 4 above). It is now possible to not specify the level in the properties file and not specify it as an annotation. If that happens then the level will be info since that is what is hardcoded in I18NLoggerFactory. What is more annoying is that I can call logger.warn and generate an info level message. b. Other than showing that you know how to use annotations I don't see what the benefit is over doing something like:

public enum LogMessages { private final LogLevel level; private final String message;

public LogMessages(LogLevel lvl, String msg) { level = lvl; message = msg; }

public LogLevel getLogLevel() { return level }; public String getMessage() { return message };

TEST0001(LogLevel.ERROR, "error message {}"); TEST0002(LogLevel.WARN, "warning message {}"};

}

I only spent about an hour looking at this so this may not be the complete set of issues I might find.

In short, this implementation is not very general purpose as it only supports a single use case.

Ralph

On Aug 22, 2009, at 10:32 PM, Takeshi Kondo wrote: