views:

563

answers:

14

A colleague of mine refactored this code:

private void btnGeneral_Click(object sender, RoutedEventArgs e)
{
    Button button = (Button)e.OriginalSource;
    Type type = this.GetType();
    Assembly assembly = type.Assembly;
    string userControlFullName = String.Format("{0}.{1}", type.Namespace, button.Name);
    UserControl userControl = (UserControl)assembly.CreateInstance(userControlFullName);
}

to this code:

private void btnGeneral_Click(object sender, RoutedEventArgs e)
{
    Button button = (Button)e.OriginalSource;
    Type type = this.GetType();
    Assembly assembly = type.Assembly;
    UserControl userControl = (UserControl)assembly.CreateInstance(String.Format("{0}.{1}", type.Namespace, button.Name));
}

saying that you don't need to create a variable if it is only going to be used once.

My response was that making once-used variables is good practice since it:

  • functions as and reduces comments (it is clear what "userControlFullName" is)
  • makes code easier to read, i.e. more of your code "reads like English"
  • avoids super-long statements by replacing parts of them with clear variable names
  • easier to debug since you can mouse over the variable name, and in the cases of e.g. PHP programming without debuggers, it is easier to echo out these variable names to get their values

The arguments against this way "more lines of code", "unnecessary variables" are arguments to make life easier for the compiler but with no significant speed or resource savings.

Can anyone think of any situations in which one should not create once-used variable names?

+17  A: 

All your reasons seem valid to me.

There are occasions where you effectively have to avoid using intermediate variables, where you need a single expression (e.g. for member variable initialization in Java/C#) but introducing an extra variable for clarity is absolutely fine where it's applicable. Obviously don't do it for every argument to every method, but in moderation it can help a lot.

The debugging argument is particularly strong - it's also really nice to be able to step over the lines which "prepare" the arguments to a method, and step straight into the method itself, having seen the arguments easily in the debugger.

Jon Skeet
Offensive? Wow, I'm trying to work out in what possible way this answer could be regarded as offensive (or spam, or abuse). Anyone want to give me a clue?
Jon Skeet
I didn't understand it, either. I was actually looking for the "No, it's not" link.
Hank Gay
Someone wanted a badge perhaps?
Georg
@gs: Yeah, either that or someone clicked it by accident and realized that it cannot be undone.
Zach Scrivena
I, too, am amazed by the offensive tag. Maybe someone is offended by your 37K+ rep?
Software Monkey
Not to worry. So long as we don't get more people doing the same, no harm done :)
Jon Skeet
@Jon: You really shouldn't say that... =P
Zach Scrivena
Hmm... 3 downvotes as well as offensive, it seems.
Jon Skeet
+4  A: 

Since the programming languages I use generally do not tell me what was null in an exception stacktrace I generally try to use variables so that no more than one item per line can be null. I actually find this to be the most significant limiter of how many statements I want to put on a single line.

If you get a nullpointerexception in this statement from your production logs you're really in trouble:

getCustomer().getOrders().iterator().next().getItems().iterator().next().getProduct().getName()

Although I agree with your thoughts, adding an extra variable can introduce an extra concept in the method and this concept may not always be relevant to the overall goal of the method. So excessive adding of variables can also increase method complexity and reduce legibility. Note the usage of excessive here.

krosenvold
I disagree. Compared to the irritatingly long line, the extra variable doesn't "introduce" an extra concept; instead, it explains the concept that is already there, by giving it a name.
Daniel Daranas
Added clarification about "excessive" and relevance to overall goal in slight edit.
krosenvold
ditto what Daniel Daranas said =)
Brock Woolf
The variable you are introducing should still be *relevant* as to the main intention of the method. Otherwise you're just adding fluff
krosenvold
OR, as I say, it should be relevant for technical reasons (NullPointerExceptions)
krosenvold
Exactly the reason why I am prolific about using Single-Use-Variables... It allows me to check each variable for null/emptiness before using it further.
Cerebrus
+25  A: 

I'm with your opinion in this case. Readability is key. I'm sure that the compiler produces the same executable in both cases, with the compilers as intelligent as they are today.

But I wouldn't claim "always use once-used variables" either. Example:

String name = "John";
person.setName(name);

is unnecessary, because

person.setName("John");

reads equally well - if not even better. But, of course, not all cases are as clear cut. "Readability" is a subjective term, after all.

Henrik Paul
+6  A: 

I'm completely with you on this one.

I especially use this if a method takes a lot of booleans, ie

public void OpenDocument(string filename, bool asReadonly, bool copyLocal, bool somethingElse)

To me this is a lot more readable:

bool asReadonly = true;
bool copyLocal = false;
bool somethingElse = true;

OpenDocument("somefile.txt", asReadonly, copyLocal, somethingElse);

..than:

OpenDocument("somefile.txt", true, false, true);
Peter Lindholm
I use "OpenDocument("somefile.txt", /*asReadonly=*/true, /*copyLocal=*/false, /*somethingElse=*/true);" tabbed if needed. Of course, sensible enums are better :)
Simon Buchan
s/tabbed/word wrapped/. Can't brain at even 10pm...
Simon Buchan
A: 

Agreed

"makes code easier to read, i.e. more of your code "reads like English"

I think this is the most important factor as there is no difference in performance or functionality on most moder managed languages

After all we all know code is harder to read than it is to write.

Karl

Karl
Unless your native language isn't English... he he. (It's late, I should prob. turn out the light and go to sleep now).
Software Monkey
+1  A: 

The both codes are exactly the same. Of course, yours is more readable, maintenable and debuggable, but, if that was the point of your colleague, his code is NOT memory less consumer.

Fabian Vilers
+2  A: 

I think it's a judgement call based on how tidy you want you code to be. I also think that both you and your colleague are correct.

In this instance I would side with you colleague based on the code you presented (for performance reasons), however as I said before it does depend on the context in which it will be used and I think your position is perfectly acceptable.

I would point out that creating variables for once used parameters can be pointless, unless they are const variables or things that you need to use in many places.

I would argue that declaring a once used variable could possible create more confusion when you are debugging if there are lots and lots of these, but one here and there is probably fine.

Brock Woolf
+2  A: 

I guess in some cases where it could have an effect on performance. In particular in this example:

for (int i1 = 0; i1 < BIG_NR; i1++)
{
  for (int i2 = 0; i2 < BIG_NR; i2++)
  {
    for (int i3 = 0; i3 < BIG_NR; i3++)
    {
      for (int i4 = 0; i4 < BIG_NR; i4++)
      {
        int amount = a + b;
        someVar[i1][i2][i3][i4] = amount;
      }
    }
  }
}

... the extra assignment might have a too big impact on performance. But in general, your arguments are 100% correct.

tehvan
Any halfway decent compiler/vm is going to optimize that away. But in this example, the var is not needed for readability anyway, since the expression would read as well or better inline.
Software Monkey
+16  A: 

Your colleague doesn't seem to be consistent.

The consistent solution looks like this:

private void btnGeneral_Click(object sender, RoutedEventArgs e)
{
    UserControl userControl = ((UserControl)type.Assembly).CreateInstance(String.Format("{0}.{1}", this.GetType().Namespace, ((Button)e.OriginalSource).Name));
}
ThomasD
Yes, that's the first thing that occurred to me... inconsistency! +1
Cerebrus
+1  A: 

Trying hard to come up with an argument against introducing new variables I'd say that when you read the code (for the first time, at least), you don't know if the variable is being used more than once. So immediately you will let your eyes scan down through the code to see if it is used in more places. The longer the function the more will you have to look to see if you can find it.

Thats's the best argument against that I can come up with! :-)

danbystrom
A: 

I'm in agreement with the majority here, code readability is key.

It's a rare line count crusader that actually writes highly readable, highly maintainable code.

Additionally, it all gets compiled to MSIL anyway and the compiler will optimise a lot for you.

In the following example, the compiler will optimise the code anyway:

List<string> someStrings = new List<string>();
for (int i = 0; i < 1000; i++)
{
    string localString = string.Format("prefix{0}", i);
    someStrings.Add(localString);
}

Rather than:

List<string> someStrings = new List<string>();
string localString = string.Empty;
for (int i = 0; i < 1000; i++)
{
    localString = string.Format("prefix{0}", i);
    someStrings.Add(localString);
}

So there's really no performance reason not to go with it in many cases.

TreeUK
+1  A: 

Creating a new variable means one more concept for the reader to keep track of. (Consider the extreme case: int b=a;c=b;) If a method is so complex - in such need of breaking up - that the extra concept is a price worth paying, then you ought to go the whole hog and split it into two methods. This way you get both the meaningful name and the smaller size. (Smaller for your original method: if it's like you say, then people won't typically need to read the auxiliary method.)

That's a generalisation, particularly in a language with a lot of boilerplate for adding new methods, but you're not going to disagree with the generalisation often enough to make it worth leaving out of your style guide.

A: 

That's how I used to code. Nowadays I tried to minimize intermediate variables. The use of intermediate variables is perfectly fine if it's immutable.

Hao Wooi Lim
+1  A: 

I'm completely with your colleague in principle, but not in this case.

The problem with throwaway variables is that they introduce state, and more state means the code is harder to understand since you don't know what effects it could have on the program's flow. Functional programming has no variables at all, exactly for this reason. So the fewer variables there are, the better. Eliminating variables is good.

However, in this particular case, where the method ends right after the variable's only use, the disadvantages of having it are minimal, and the advantages you mention are probably more substantial.

Michael Borgwardt
+1 for the functional programming mention in the context of avoiding state, I was listening to a podcast on F# and couldn't figure out why variables were such a different animal in that language (I understand that all "variables" are constants in F# which is an oxymoron and therfore confusing).
Edward Tanguay