Alon Bar-Lev has posted comments on this change.

Change subject: engine: Get Gluster Servers query
......................................................................


Patch Set 10: (2 inline comments)

For some reason these were not submitted, sorry.

If you like I can just go ahead and fix it.

....................................................
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) {
You can put each if block within its own function... each code block within own 
function... For every if you add function heder, and function suffix, your code 
grow exponential.

It is sense to add a new function to break large chunk of codes even if it is 
used only once.

It is sense to add a new function for code reuse.

Not that I will reject this function, only that I really don't understand why 
it is required.

Anyway, if you going to keep it, I expect you add parameter of which 
fingerprint to validate, move the getParameters().getFingerprint() to the place 
you call the function.
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();
Harm - no.

But unexpected? yes.

This discussion shows that... :)

Please move out the authenticate.
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 <dgo...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Dhandapani Gopal <dgo...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Selvasundaram <sesub...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to