views:

343

answers:

8

This is something that I have always wondered about, but never bothered to profile.

Is it more efficient to assign a value to a temp variable, than to keep using that value. An Example may be clearer:

string s = reader.GetItem[0].ToString();
someClass.SomeField  = s;
someOtherClass.someField = s;

OR

someClass.SomeField  = reader.GetItem[0].ToString();
someOtherClass.someField = reader.GetItem[0].ToString();

My initial thought would the top example would be more efficient as it doesn't have to access the Item collection or call ToString.

Would be interested to hear other peoples ideas, or definitive answer either way.

+15  A: 

The compiler cannot know if the expression on the right-hand-side has side-effects, so it must re-evaluate it if you code it twice. Hence the first is more efficient in the sense that it will not re-do the GetItem & ToString calls.

So if you the programmer know that these calls are pure/idempotent, then you should write the code the first way.

Brian
A: 

Well assuming that the ToString function not just hands out a reference to some pre-made internal object, it must be pretty clear that the first version, where only one call to it is being made, will be the fastest. However, if this is a performance issue you should care about or not is a completely different story.

One issue to do think bout, however, is that the second example might render different results in the first and second line if the item is accessed/changed simultaneously from another thread.

Johann Gerell
+6  A: 

As Brian said, the first way will be more efficient, though whether it makes much difference in the real world depends on how expensive the duplicated functions are, and how frequently this piece of code as a whole is called.

However, as well as being more efficient, the first way better indicates intention - that you mean to assign the same value to the two things. It also aids maintainability because if it needs to change, you only need to change it in one place. For me, both of these are typically more important than efficiency.

Greg Beech
Some will probably say they hate this, but I will often do the following to make it clear that the multiple variables are being initialized to the same value.oneVar = anotherVar = someFunctionCall();
grantwparks
What .net languages is that legal in?
recursive
It's valid in C# at least. I'm divided on the suggestion from @grantwparks - one the one hand, it very directly expresses the intent to initialize both to the same value. On the other hand, I've always disliked the practice as hard to read. Food for thought ...
Bevan
A: 

I just have habit of storing a value in a local variable if I'm going to be using it more than once. Usually, though, it's because I prefer the compactness of the code rather than being too concerned about efficiency -- although, I'd definitely do it if using it repeatedly in a loop.

Sometimes, I'm inconsistent and will just retype, especially if just using an accessor and not calling a method that requires computation.

tvanfosson
A: 

Here's my rule of thumb. When in doubt, profile your code. An optimizing compiler can remove a lot of code thus making your code run faster.

Two facts must also be considered:

  • .NET allocates memory very quickly.
  • Excessive garbage can slow down your application due to additional page faults, poor locality of reference, excessive garbage collection.

This implies that under a cursory observation, your code can be very fast, but if you're generating a lot of garbage, your program's performance will suffer as it runs for a while.

Tim Stewart
A variable adds no garbage; there is no additional heap allocation for the "s" scenario. Re-using the indexer might, however, since the .ToString() will probably result in a different string object (unless the item was already a string).
Marc Gravell
A: 

Readability matters. What is the use of "s" named variable?

Also, instead of using [0], a field name will make more sense.

shahkalpesh
Using the name would also make it slower; a balancing act.
Marc Gravell
Just an arbitrary example - wasn't real code
KiwiBastard
+2  A: 

There is another option - composite assignment:

someOtherClass.someField  = someClass.SomeField  = reader.GetItem[0].ToString();

With this usage the compiler will evaluate reader.GetItem[0].ToString() once, and use it to assign to both members (it does not use the getof someClass). It does this by duplicating the value on the stack (which doesn't need an explicit local).

Very efficient, but to be honest I wouldn't get too excited about the original with a variable.

Marc Gravell
A: 

There are other issues to consider. If the use of s, to assign to other variables, is separated from where s is initialized by more than a couple lines of code, you open the possibility that someone else will come along and add code later that alters the value of s between its uses, or somehow branches around the original assignment to s.

One thing I see a lot of is the assignment of a function return to a variable even when that value is only being used in one place, and I hate that, because (and it doesn't matter what that variable is named) that inevitably leads to having to go find where that variable was assigned to know what it really represents. Assigning the return value of the function directly to what is going to use it, explicitly indicates what's going on.

There is a faction in programming that believes in "variable-less programming" (in the flavor of the famous "goto-less programming" paper decades ago). For example, XSL, while it does have "variables", they are not mutable after their initial assignment within a single scope. This, some say, is what helps guarantee no unintended side-effects.

grantwparks