[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[rafaelweingartner]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---