Shireesh Anjal has posted comments on this change.
Change subject: engine: Get Gluster Servers query
......................................................................
Patch Set 10: (2 inline comments)
replies to couple of Alon's comments in-line.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQuery.java
Line 93: }
Line 94: }
Line 95: }
Line 96:
Line 97: private void validateFingerprint(SSHClient client) {
I agree with you - putting each if condition in a separate function would be
like following the suggestion blindly, without considering "if it makes sense"
part.
The simplest way to think about whether a piece of code should be extracted
into a function is, whether it performs a logical unit of work. Validating the
fingerprint definitely satisfies this, and it makes the code inside
executeQueryCommand() far more readable. Breaking large code into multiple
chunks and code reuse are also valid cases, but definitely not the only ones.
I like your suggestion of taking the fingerprint as an argument - it would make
the code even more readable. I suggest Dhandapani to implement this.
Line 98: if
(!getParameters().getFingerprint().equals(getFingerprint(client))) {
Line 99: String errMsg =
Line 100: "SSH Fingerprint of server " +
getParameters().getServerName()
Line 101: + " did not match expected fingerprint "
+ getParameters().getFingerprint();
Line 131: client.setUser("root");
Line 132: client.setPassword(getParameters().getRootPassword());
Line 133:
Line 134: try {
Line 135: client.authenticate();
I don't think it is such a big issue after renaming the method to
authenticateAndExecuteCommand().
In fact, it is interesting to note here that the piece of code to be moved out
into a separate method
- is very short
- does not break any large piece of code into two
- does not offer reuse as it is called only once in the class
In spite of these, it is a valid candidate to be moved into a separate
function, because it performs a logical unit of work. From that perspective,
I'm with you. The only thing I don't like too much about it is the need to add
another try-catch block, that will do nothing more than what is being done here.
Having said this, I would leave the decision between the developer and you.
While I don't see much wrong with the existing code, I'm not strongly against
having an authenticate method either.
Line 136: client.executeCommand(command, null, out, null);
Line 137: cliOutput = new String(out.toByteArray(), "UTF-8");
Line 138: } catch (Exception e) {
Line 139: String m =
--
To view, visit http://gerrit.ovirt.org/7584
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic69a9a48bf227c8fa805c8aa9c4f08fa36ea9425
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Selvasundaram <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
