Ok, Lauret, Mandy,
Here are the final touches:
https://dl.dropboxusercontent.com/u/101777488/jdk8-tl/PlatformLogger/webrev.07/index.html
Changes from webrev.06:
- renamed back Level -> LevelEnum
- renamed JavaLogger -> JavaLoggerProxy
- moved javaLevel(int) static method from LevelEnum to JavaLoggerProxy
and made it private (only used there)
- consistent use of variable name 'level' to refer to PlatformLogger's
int level value
- consistent use of variable name 'levelEnum' to refer to LevelEnum member
- consistent use of variable name 'javaLevel' to refer to
java.util.logging.Level instance
- consistent use of variable name 'javaLogger' to refer to
java.util.logging.Logger instance
- renamed PlatformLogger.newJavaLogger() private method ->
PlatformLogger.redirectToJavaLoggerProxy()
- renamed PlatformLogger.logger private field -> PlatformLogger.loggerProxy
- some additional comments
I think that these changes make code more consistent and self-explanatory.
What remains is a possible rename from: javaLogger, javaLevel,
JavaLoggerProxy -> julLogger, julLevel, JulLoggerProxy if that's the
final decision.
Regards, Peter
On 03/22/2013 01:26 PM, Peter Levart wrote:
On 03/22/2013 11:34 AM, Laurent Bourgès wrote:
Peter,
You've beaten me! I have been preparing them too ;-) ...
Ok I definitely stop working on the code and let you do it.
Ok. I'll make the final touches, incorporating also comments and
changes from your code...
I also did some some renames, that I think make the code more
consistent:
- LevelEnum -> Level (the code is not dependent on
java.util.logging.Level, so the name can be reused, its private
anyway)
- julLevel -> javaLevel (javaLevel / JavaLogger)
- LevelEnum.forValue -> Level.valueOf (Mandy)
- JavaLogger.julLevelToEnum -> JavaLogger.javaLevelToLevel
For consistency and clarity, I would prefer having following conventions:
- int levelValue (= PlatformLevel as int) and not int level (conflict
with Level enum ...)
I think that PlatformLogger public API should stay as is. Public
method's parameter names are just called 'level' and values of public
constants are expected to be passed to them. There are only two places
where 'level' is the name of a local variable of type Level (and not
int) and at both places there is not conflict, since there's no 'int
level' variable in scope.
I renamed LevelEnum to Level because the following most frequently
used pattern looks strange:
LevelEnum.javaLevel(int)
Neither parameter nor the result has anything to do with LevelEnum
directly.
But if we move the javaLevel(int) method out of the Level enum into
the JavaLogger, then Level can be renamed back to LevelEnum (or
anything else?).
- julLevel / julLogger: more explicit than javaLevel / javaLogger
(java means everything ... but jul means java.util.logging and
javaLogger is in conflict with JavaLogger class)
But javaLogger is not in conflict with javaLevel. I suggest renaming
JavaLoger class to JavaLoggerProxy, so we would have:
Object javaLogger // holding java.util.logging.Logger instance
Object javaLevel // holding java.util.logging.Level instance
class JavaLoggerProxy // a specialization of LoggerProxy, delegating
to javaLogger
If 'jul' is a prefered prefix to 'java', I suggest renaming all 3:
julLogger, julLevel, JulLoggerProxy. I don't have a preference for
either, so perhaps Mandy, Laurent or anybody else can express them...
Regards, Peter
Other changes (to webrev.05):
- removed the occurrence counts in switch comments (as per
Mandy's suggestion)
- made LoggerProxy and JavaLogger private
- fixed double-read of volatile LoggerProxy.levelValue in
LoggerProxy.isLoggable()
- added static Level.javaLevel(int value) shortcut (Mandy)
I also updated the test to exercise the correctness of mappings.
Well done.
cheers,
Laurent
