Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172347935
@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 [email protected] or file a JIRA ticket
with INFRA.
---
