Vojtech Szocs has posted comments on this change. Change subject: userportal,webadmin:change Translator to interface ......................................................................
Patch Set 8: (4 comments) http://gerrit.ovirt.org/#/c/26596/8/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/EnumTranslator.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/EnumTranslator.java: Line 22: Line 23: @Override Line 24: public String translate(Enum<?> key) { Line 25: if(key == null) { Line 26: logger.log(Level.INFO, "trying to localize null, probable error. " + This could be logged as a WARNING instead of INFO. Line 27: "Exception is not thrown, returning '"+ constants.notAvailableLabel()+"'", new RuntimeException()); Line 28: return constants.notAvailableLabel(); Line 29: } Line 30: Line 23: @Override Line 24: public String translate(Enum<?> key) { Line 25: if(key == null) { Line 26: logger.log(Level.INFO, "trying to localize null, probable error. " + Line 27: "Exception is not thrown, returning '"+ constants.notAvailableLabel()+"'", new RuntimeException()); Please format the code to comply with general conventions (i.e. space before and after "+" etc). It seems a bit weird to log INFO message with exception attached to it. Please consider logging the message as WARNING: logger.warning(" ... "); I assume that "new RuntimeException()" used here is to expose the exact stacktrace in GWT debug console, but I wouldn't use it - developers should be able to trace the source from log message itself. Line 28: return constants.notAvailableLabel(); Line 29: } Line 30: Line 31: try { Line 46: if (e != null) { Line 47: logString += " " + e.getLocalizedMessage(); Line 48: } Line 49: Line 50: logger.info(logString); This could be logged as a WARNING instead of INFO. Line 51: return key.name(); Line 52: } Line 53: Line 54: private String keyToTranslate(Enum<?> key) { Line 52: } Line 53: Line 54: private String keyToTranslate(Enum<?> key) { Line 55: String className = key.getDeclaringClass().toString(); Line 56: String classNameWithoutPackage = className.substring(className.lastIndexOf(".")+1,className.length()); //$NON-NLS-1$ Please format the code to comply with general conventions (i.e. space before and after "+" etc). Line 57: Line 58: return classNameWithoutPackage + "___" + key.name(); Line 59: } Line 60: -- To view, visit http://gerrit.ovirt.org/26596 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib018c1faf0c2e1ebaa81217d5e3696d9c8de20cf Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
