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 [email protected] or file a JIRA ticket
with INFRA.
---
