[GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1287#issuecomment-172196150
  
    @DaanHoogland, 
    I reviewed your code and I have a few suggestions/questions.
    
    Are we really going to use final in all of our variables/attributes? I 
personally do not like to use final declarations, I use them only if needed; 
they tend to make extensions harder (sometimes).
    
    The following comments are about “SecurityGroupRulesCmd” class:
    At lines 73, 129, 140 and maybe others, why not initialize the ArrayList 
when you declare the attribute, this way it is always ready for use. I 
personally like to work this way, initializing List, Maps and others at the 
moment of variable declaration.
    
    I would also suggest improving the java doc at method 
“stringifyRulesFor” to make it clear of its purpose and how it works.
    
    The comment of method “compressCidr” should be converted to proper java 
doc style. I would also use the "/" magical character as a constant.
    At line 206, I would suggest you extracting the code to a method and 
changing the contracted “IF” to a normal if structure, improving 
readability.
    
    I also recommend changing the comment of “compressStringifiedRules” 
method to proper java doc style. If you do that, please extract the comment at 
lines 221 and 222 to the java doc.
    
    I recommend you removing the comment of line 251. If there is something to 
be said about that method, that should be done at the Java doc.
    
    The following comments are about “SecurityGroupRulesCmdTest” class.
    At the test if the “setUpBeforeClass” is not used, I suggest removing 
it. I think the best philosophy is to add code only when needed.
    
    Why does the class “SecurityGroupHttpClient” is being marked as 
completely changed? It seems that nothing has been changed there. Just some 
small adjusts at line 75 and 76
    
    I believe that is all. There are other suggestions to add some more Java 
doc and tests, but let's keep them to another PR in the near future.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---