So, your official position is that declaring a variable as constant is typically pointless because some future programmer can always change the variable to not be constant. :)
In this case, to appease both parties (you and Wido), I’ll leave it final, but add a comment to explain why it’s final. It should end up being a moot point in 4.12 as I plan to change the replaceStorage method to create a shallow copy of the passed-in map and mutate it as need be. Then, we won’t even need this Boolean. > On Jan 20, 2018, at 9:58 AM, Rafael Weingärtner <rafaelweingart...@gmail.com> > wrote: > > No setters help to make an object immutable, but in Java we have > reflection, and the only way to really avoid changing a property is using > the final modifier. However, even when using final, it is possible to do so > by manipulating the byte code of the class that describes the object and is > loaded to the JVM. This is what PowerMock does to deal with static and > final method/variable mocking. > > On Sat, Jan 20, 2018 at 2:40 PM, Tutkowski, Mike <mike.tutkow...@netapp.com> > wrote: > >> “For instance, when we have to design/create a library and we want to make >> sure an object is immutable.” >> >> According to your argument, though, you don’t need final here either: just >> make sure to never provide setters or change those values (and document). >> >>> On Jan 20, 2018, at 9:37 AM, Tutkowski, Mike <mike.tutkow...@netapp.com> >> wrote: >>> >>> Personally, I’m OK with it either way here. If @wido reads what you >> wrote and asks me to change it back to the way I wrote it initially, I’m >> happy to do so. >>> >>> I believe he sees the explicitly declared constant here not only as a >> protection against ourselves, but to prevent a future programmer from >> easily making a mistake. At least now, if this future programmer changes >> the value, the compile will fail and he/she will be forced to think through >> the repercussions of doing so. >>> >>>> On Jan 20, 2018, at 9:30 AM, Rafael Weingärtner < >> rafaelweingart...@gmail.com> wrote: >>>> >>>> Never is a strong word. In any case, let it be if you believe it is >> going >>>> to provide benefits… >>>> >>>> I believe `final` modifier should be used in certain situation when it >> is >>>> truly required. For instance, when we have to design/create a library >> and >>>> we want to make sure an object is immutable. Then, we configure all of >> its >>>> variables as final. Now, declaring a variable final in the middle of a >>>> method to protect against ourselves…. It does not seem to bring much >>>> benefit against future mistaken/accidental changes. It is always >> possible >>>> for the hypothetical future programmer to remove the final and change >> the >>>> variable. Perhaps a better documentation for the method explaining this >>>> situations could bring more benefits. A single variable declared as >> final >>>> does not give a hint for people on why it was made final. >>>> >>>> On Sat, Jan 20, 2018 at 2:08 PM, Tutkowski, Mike < >> mike.tutkow...@netapp.com> >>>> wrote: >>>> >>>>> Perhaps your argument against the final keyword is that it should >> never be >>>>> used? If so, you and @wido can debate that and let me know which way >> you’d >>>>> like this bit of code to end up. >>>>> >>>>>> On Jan 20, 2018, at 9:05 AM, Tutkowski, Mike < >> mike.tutkow...@netapp.com> >>>>> wrote: >>>>>> >>>>>> It’s pretty much the reason why the final variable exists in Java: to >>>>> make a variable a constant so no one accidentally changes it. @wido >> just >>>>> wanted the code to protect against the variable being changed...that’s >> all. >>>>>> >>>>>>> On Jan 20, 2018, at 8:25 AM, Rafael Weingärtner < >>>>> rafaelweingart...@gmail.com> wrote: >>>>>>> >>>>>>> Changed by a future programmer that may develop some changes in your >>>>> code? >>>>>>> Because, if you do not code any other assignment to that variable, it >>>>> will >>>>>>> never happen. >>>>>>> >>>>>>> On Sat, Jan 20, 2018 at 1:16 PM, Tutkowski, Mike < >>>>> mike.tutkow...@netapp.com> >>>>>>> wrote: >>>>>>> >>>>>>>> @wido just wanted to make sure the Boolean variable didn’t >> accidentally >>>>>>>> get changed later since it’s critical it keep that value. >>>>>>>> >>>>>>>>> On Jan 20, 2018, at 8:14 AM, Tutkowski, Mike < >>>>> mike.tutkow...@netapp.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> I made it final per @wido’s comment (in #2415). How’s about you >> guys >>>>>>>> hash it out and get back to me. :) >>>>>>>>> >>>>>>>>>> On Jan 20, 2018, at 8:06 AM, GitBox <g...@apache.org> wrote: >>>>>>>>>> >>>>>>>>>> rafaelweingartner commented on a change in pull request #2416: >>>>>>>> CLOUDSTACK-10244: KVM online storage migration fails >>>>>>>>>> URL: https://github.com/apache/cloudstack/pull/2416# >>>>>>>> discussion_r162784630 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ########## >>>>>>>>>> File path: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/ >>>>>>>> resource/wrapper/LibvirtMigrateCommandWrapper.java >>>>>>>>>> ########## >>>>>>>>>> @@ -132,8 +132,9 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to >>>>>>>> v1.0.0. >>>>>>>>>> vmsnapshots = libvirtComputingResource. >>>>>>>> cleanVMSnapshotMetadata(dm); >>>>>>>>>> >>>>>>>>>> Map<String, MigrateCommand.MigrateDiskInfo> >>>>>>>> mapMigrateStorage = command.getMigrateStorage(); >>>>>>>>>> + final boolean migrateStorage = MapUtils.isNotEmpty( >>>>>>>> mapMigrateStorage); >>>>>>>>>> >>>>>>>>>> Review comment: >>>>>>>>>> This variable does not need to be final. It does not bring any >>>>>>>> benefits here. >>>>>>>>>> >>>>>>>>>> You needed to assign the result of the first `MapUtils.isNotEmpty( >>>>> mapMigrateStorage)` >>>>>>>> because the method `replaceStorage` might change the status of >>>>>>>> `mapMigrateStorage`, right? >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------- >>>>>>>>>> This is an automated message from the Apache Git Service. >>>>>>>>>> To respond to the message, please log on GitHub and use the >>>>>>>>>> URL above to go to the specific comment. >>>>>>>>>> >>>>>>>>>> For queries about this service, please contact Infrastructure at: >>>>>>>>>> us...@infra.apache.org >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> With regards, >>>>>>>>>> Apache Git Services >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Rafael Weingärtner >>>>> >>>> >>>> >>>> >>>> -- >>>> Rafael Weingärtner >> > > > > -- > Rafael Weingärtner
