[GitHub] cloudstack pull request: Fix Sync of template.properties in Swift

[rafaelweingartner]

Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1331#issuecomment-210018309
  
    @syed, I totally understand that you do not have much experience with Java 
development and that you were following the practices already put in place. The 
problem is that we have so many cases of what not to do. I find it very good 
that you are willing to learn and help us improve the ACS code base.
    
    So let’s go to the work.
    
    Sure I changed the variable “logger” to protected, I needed to access 
it on a test case, so I changed it to an access delimiter that I could use; you 
should always try to use the most restrictive one, but at the end in Java those 
access delimiters can always be bypassed using Reflections. The protected 
delimiter states that children’s of that class and classes at the same 
package will have access to that variable/method.
    
    I also removed the “final” because I needed to change that variable to 
the test with a mocked one. The final delimiter would not allow that; 
therefore, I had to remove it. That is why I would normally avoid the use of 
final everywhere, it can complicate things. I would only use final where I 
really want to be strict and not allow a variable change on the fly.
    
    I removed the static to transform that variable into an “instance” 
attribute; those objects are spring beans, and they are working as singletons, 
there will be no more than one instance of them. That is why I see no reason to 
use that attribute as a class field.
    Now, you could go over all of the classes that extend 
“NfsSecondaryStorageResource”, they would have their own “logger” 
variable, then you could remove the “s_logger” they are using and use the 
one inherited. Additionally, you would be using a more common name for that 
variable that is “logger”. 
    
    For your second batch of questions, I prefer to use the most restrictive 
delimiter access possible. I always start with private and then I go opening 
them on a need basis. I have no idea why someone would do different, such as 
using “public static final” when it is not needed. ACS has a lot of code 
that should not be taken as reference. In doubt, you can always call for help; 
there are folks here that are great devs.
    
    
    



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