views:

288

answers:

6

I have been looking through some code on an open source project recently and found many occurrences of this kind of code:

class SomeClass
{
    private int SomeNumber = 42;

    public ReturnValue UseSomeNumber(...)
    {
        int someNumberCopy = this.SomeNumber;
        if (someNumberCopy > ...)
        {
            // ... do some work with someNumberCopy
        }
        else
        {
            // ... do something else with someNumberCopy
        }
    }
}

Is there any real benefit to making a copy of the instance variable?

A: 

The only benefit I see is for having a "readonly version" of the variable just for that method

rossoft
A: 

no. leave this work for optimizer.

Andrey
+6  A: 

No unless you don't want to change the value of SomeNumber and you intend on updating someNumberCopy. Like if you were going to loop the number of times and were going to decrement someNumberCopy down to zero to keep track of the count.

I suppose copying the variable like that could protect you from some outside function altering SomeNumber and changing it without your knowledge while performing an operation. I could potentially see this if the class was supposed to be used in a multi-threaded application. Maybe not he way I would go about it, but that could have been the author's intent.

Kevin
+1  A: 

Is the // ... do some work with someNumberCopy doing something with someNumberCopy? Is it changing the value? Is it being passed to a method that might change the value?

If not then no, there is no benefit.

David Basarab
+8  A: 

Possibly this is part of multi-threaded program. Though this code is not thread-safe, it ensures that once copy variable is assigned, it is not changed by another threads, and all function code after this runs consistently.

Similar code with events becomes critical in multi-threaded environment:

MyEvent e = this.myEvent;

if ( e != null )
{
    e();
}

Here, without making a local copy, it is possible to get null-pointer exception, if event becomes null after testing for null, and before invoking.

Alex Farber
I didn't even think about the multi-threaded when I was looking at the code, so thank you for your answer. Having looked back at it now, it turns out the actual usage is that the instance variable is already readonly and set by the constructor, and the value is not changed. It now looks to me like this may have been done to change the name of the variable so the code reads easier.
NickLarsen
+1  A: 

It could be useful if this.SomeNumber can be modified by another thread, but for the duration of this method it is vital that the someNumberCopy cannot be changed (it can be out of sync with SomeNumber) then this might be viable, otherwise I don't see a reason for it.

Davy8