cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rafaelweingartner <>
Subject [GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Date Sun, 17 Jan 2016 16:37:56 GMT
Github user rafaelweingartner commented on the pull request:
    @DaanHoogland, that is great.
    Those points I lifted up were merely suggestions on how to improve the code a little bit
more. Actually, in most PRs I review the code normally is ok, I just point out some aspects
that can be improved.
    I understand your point on using the final modifier. Since ACS is not a framework, I can
agree with you here. I will start using them from now on.
    We only diverge at the point regarding Java docs, and that is fine. I know that are some
other development philosophies that do not like to use documentation, I understand their point.
I also noticed that some people here do not like to use much Java doc and sometimes they confuse
java doc with block comment.
    I believe that all of Apache libraries/software I know are heavily documented, in a page
that presents the use of the library/software and at the code. That is one of the reasons
why I love apache Java libraries/software, because of their documentation, from which I can
understand and use the software without needing to rely on someone else. IMO ACS should have
more documentation on its code for two reasons.
    First, it gives a purpose to the method or class. It forces people to think on what they
will code into a method and stick to what was planned, instead of planning and creating code
on the fly which in some cases lead to big chunks that are hard to maintain and refactor.
    Second, Java docs facilitate to new developers to really understand the behavior and purpose
of classes and methods. That also helps when developing new code; with proper documentation,
I find it easier to uncover the right place to place a new method/function.
    About documentation on private methods, I use them (write them sometimes). I also have
used them (read) and find them quite useful in some components I use. Apache libraries such
as Apache Commons use them. I understand that there are self-explanatory methods which do
not need documentation. However, given the complexity of ACS, I believe that proper java doc
would facilitate to new devs. I am not saying that we should start documenting everything.
When I suggest the addition of a Java doc or a test case, it is merely a tentative to improve
our code base. I see the process of creating Java docs the same as writing test cases, it
takes time and a change in the mindset to get used to them. 
    Your code is great, I would just suggest some more improvement at the new method you created
that is called “represent”, I believe the best name for the boolean variable should be
something like, “shouldCompress” or” hasToCompress”. The name of the method does not
sound descriptive to me, but if you guys are ok with it, I am too.
    Giving all of that, it is a LGTM from me.

If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at or file a JIRA ticket
with INFRA.

View raw message