cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rafaelweingartner <...@git.apache.org>
Subject [GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Date Sat, 16 Jan 2016 11:57:40 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message