cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From cristofolini <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Date Thu, 14 Jan 2016 22:14:53 GMT
Github user cristofolini commented on the pull request:

    https://github.com/apache/cloudstack/pull/1310#issuecomment-171798749
  
    Thanks for taking the time to add the descriptions and refactoring, @nvazquez. It has
helped readability quite a bit. :)
    
    Now, in case you're interested in improving this code *even further*, may I suggest breaking
down the `postVideoCardMemoryConfigBeforeStart` method a bit more? I like what you did separating
it into two, but there are still quite a few conditionals within each other and that makes
the whole thing look convoluted, not to mention adding up to cyclomatic complexity. Bonus
points for adding javadoc style descriptions to the new methods you create like you did to
the existing ones.
    
    My last suggestion would be to write some test cases (unit tests) for the methods you've
created. After that, as far as I can assess, this PR will be perfect. :)


---
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