Juan Hernandez has posted comments on this change.
Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................
Patch Set 1: (8 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/MinaInstallWrapper.java
Line 26: static Log _log = LogFactory.getLog(MinaInstallWrapper.class);
Regarding 1: I think there is no need for such a convention, if you use proper
names for things they speak for themselves. Also this is not a common practice
in Java. Some people do it, but I really prefer that we don't. If there are
situations where you really need to make the difference stand out just use
"this.whatever".
Regarding 2: If you don't put any access modifier what you get is "package
private". That means that any class in the same package can directly read and
write the fields, and that means that if I want to break your code (probably by
accident) I just have to write a class in the same package and start messing
with your internal details. Private should be the default, but it isn't, so we
need to be explicit and put the "private" access modifier.
Line 35: _port = port;
I do mind :-) , so if you don't ...
Line 38: Credentials _prepareCredentials(String rootPassword) {
No flexibility is lost, but some safety is won. If you later need to call one
of those "private" methods from outside the class you just need to think if
that will break your internal implementation and then change "private" to
"protected" or "public" as required.
Regarding the "_" I think the same that I think about using it in fields.
Line 178: catch (Throwable t) {
In general you should not try to catch things that you are not prepared to
handle. For example, if there is a OutOfMemoryException here will your handling
be correct? Not really: you ran out of memory and you are handling it creating
several new objects.
Line 185: _log.debug(m, t);
I might be picky about the exceptions, it is one of my obsessions, I apologize.
I prefer the information in the log twice or three times rather than none. But
other people may (and does) have different opinions.
I agree that "t" and "m" are a matter of taste. I am just making suggestions
using my own style beliefs :-) , which my be wrong, of course.
Line 207: _log.debug("wrapperShutdown enter");
Enter/leave is okay, what is redundant is the name of the method, that is
already provided by the %M conversion character in log4j, if needed, although I
have to admit that it is expensive to calculate and we don't use it currently.
Line 265: class MessageOutputStream extends OutputStream {
That is ok, I don't think it is bad, just very uncommon, you will have people
scratching their heads and asking why do you need such a thing.
Line 287: m = e.getMessage();
It is in strange situations that shouldn't have happened where you most need
the information provided by the stack trace.
--
To view, visit http://gerrit.ovirt.org/6722
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I50ba60f2db364114907485da3074feb714615e0c
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
