On Sat, Jan 16, 2016 at 12:57 PM, rafaelweingartner <g...@git.apache.org> wrote:
> 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). > Making the extension more consious is the purpose of making all final. Harder is actually an aid for a dev in checking all places a var needs to mutable. I do think we should make vars as final as possible. > > 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. > at those lines, the contents of the var might be replaced as well as initialized. > > I would also suggest improving the java doc at method > “stringifyRulesFor” to make it clear of its purpose and how it works. > I am with @mferreira on this. If comments are needed, what is actually needed is improvement of the code. This javadoc was automatically created and the method is private. I will remove the javadoc. > The comment of method “compressCidr” should be converted to proper > java doc style. I will rename the method to something more descriptive instead. > I would also use the "/" magical character as a constant. > will do. > 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. > makes sense but I will have a look at a good solution. > > 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 will have a second look at the comments but these are beyond the scope of this PR. > > 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. > sure, but please at a PR yourself, this one is definately beyond the scope of this PR, which is the removal of duplicate code to address a sonarqube warning. > > 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. > valid point, will do. > > 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 > This is usually due to line endings. I am inclined to ignore this. The class is small and the result is fine. > > 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. > test yes, javadoc only if the title of the non-private method can not be made to be descriptive enough. > > --- > 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. > --- > -- Daan
