cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: [GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Date Sun, 17 Jan 2016 14:15:21 GMT
On Sat, Jan 16, 2016 at 12:57 PM, rafaelweingartner <git@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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>



-- 
Daan

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message