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: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Date Wed, 20 Jan 2016 21:06:59 GMT
Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1348#issuecomment-173358933
  
    Hi @DaanHoogland,
    
    I would only suggest you extracting those magic numbers at line 97 to constant variables
(using some descriptive names).
    
    I also have a doubt, 
    Are we using that “@author” directive? Such as the one you have at line 26 of “LibvirtMigrateCommandWrapperTest”
    
    BTW: I really liked the “replaceIpForVNCInDescFile” method. Very nice and descriptive
method name, comprehensive java doc, test cases and the method itself is not complicated.
I believe that should be our code quality goal. Congratulations !


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