Eli Mesika has posted comments on this change. Change subject: tools : engine-config is over writing the previous values ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/25762/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigCLIParser.java: Line 93: getConfigAction().equals(ConfigActionType.ACTION_SET) : || getConfigAction().equals(ConfigActionType.ACTION_MERGE The following appears 3 times in this file , worths wrapping as a bool function : getConfigAction().equals(ConfigActionType.ACTION_SET) || getConfigAction().equals(ConfigActionType.ACTION_MERGE http://gerrit.ovirt.org/#/c/25762/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/config/EngineConfigLogic.java: Line 408: debug Message should be declared above as other messages with teh relevant place holders for parameters Why not using debugFormat instead and save all those '+' concatenations ? Yes, I know it exists already in the code , but for new code at least we must be better (and I think we have to submit a separate patch issuing that after pushing this one in...) Line 422: Line 423: private String concatenatedValue(String valueToAppend, String currentValue, String delimiter) { Line 424: String retValue = currentValue; Line 425: if (!Arrays.asList(retValue.split(delimiter)).contains(valueToAppend)) { Line 426: if (!retValue.endsWith(delimiter)) { seems as nested if can be omitted and replace with a simple && condition with the first one Line 427: retValue += delimiter; Line 428: } Line 429: retValue += valueToAppend; Line 430: } Line 423: private String concatenatedValue(String valueToAppend, String currentValue, String delimiter) { Line 424: String retValue = currentValue; Line 425: if (!Arrays.asList(retValue.split(delimiter)).contains(valueToAppend)) { Line 426: if (!retValue.endsWith(delimiter)) { Line 427: retValue += delimiter; Please use StringBuilder here Line 428: } Line 429: retValue += valueToAppend; Line 430: } Line 431: return retValue; Line 425: if (!Arrays.asList(retValue.split(delimiter)).contains(valueToAppend)) { Line 426: if (!retValue.endsWith(delimiter)) { Line 427: retValue += delimiter; Line 428: } Line 429: retValue += valueToAppend; same Line 430: } Line 431: return retValue; Line 432: } Line 433: Line 450: help Please move message to messages definitions section in the start of the file (final static ....) -- To view, visit http://gerrit.ovirt.org/25762 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c70539f47c509e3b8c23b1aa3de41bead36c1b4 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
