[GitHub] cloudstack pull request: [4.7] vmware: improve support for disk co...

[GabrielBrascher]

Github user GabrielBrascher commented on the pull request:

    https://github.com/apache/cloudstack/pull/1365#issuecomment-176495485
  
    @bhaisaab Could you please do the following changes?
    
    At **VirtualMachineVolumeChainInfo** class:
    **1 -** create Javadoc blocks (would be nice to document all the class, but 
I think that at least the class and the "getControllerFromDeviceBusName" method 
deserves a documentation);
    **2 -** simplify the **if** conditional at line **43**. Test with "( 
(StringUtils.isEmpty) || (!this.diskDeviceBusName.contains(":")) )" condition. 
The StringUtils.isEmpty method already checks if the String is empty("") or 
null 
(https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html).
    **3 -** create test for the getControllerFromDeviceBusName method.
    
    At **UserVmManagerImpl** class:
    **1 -** document the "persistDeviceBusInfo(UserVmVO vm, String 
rootDiskController)" method (lines **5458-5466**).
    **2 -** use the "StringUtils.isEmpty()" with the condition at line 
**5460**, instead of "(existingVmRootDiskController==null) || 
(existingVmRootDiskController.isEmpty())".
    **3 -** create test for "persistDeviceBusInfo(UserVmVO vm, String 
rootDiskController)" method.
    
    **VolumeApiServiceImpl**:
    **1 -** Remove “**_**” from variables names (lines **200**, **252**, 
**267**): private variables with “**_**” at the beginning is common in C++ 
but not in Java.
    
    Thanks.


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