Variable declaration closer to usage Vs Declaring at the top of Method [closed]

Question

As part of our sprints, we do peer review each others code.

Here is the code that I am reviewing.

public void SendEmail()
{

    string emailAddress = string.Empty;
    string managerEmailAddress = string.Empty;

    if (//condition)
    {
        //do something
        //Retrieve emailAddress and managerEmailAddress
        EmailMessage.To = emailAddress;
        EmailMessage.CC = managerEmailAddress;
    }
}

The suggestion from Resharper was to have the string declarations closer to the usage. However, my colleague suggest that it is not a good idea to have them closer to the usage and have them declared right at the beginning. And, I sort of agree with him because for methods where the variables are used in multiple places with different values being assigned depending upon conditions...it would make sense to have them all grouped right at the top but for smaller methods, where there is just one if condition, stick them along with the usage (for instance the above mentioned code).

What do you think?

Answer 1

I agree with Guffa and dreza, variables should only be declared at an outer scope if it is really really necessary as doing so is confusing and error prone. Also, if a method is long enough that having the variable declaration at the top means it isn't close to it's use then it could probably benefit from refactoring into separate methods. Doing this will also help readability because you'll have method names explaining what's happening.

Some examples (quotes as this is only my opinion):

1: "Bad" example

As an outside coder, if I saw this I would wonder why the variables are declared at the outer scope and would waste time worrying there was a special reason for it.

string emailAddress = string.Empty;
string managerEmailAddress = string.Empty;

if (//condition)
{
  emailAddress = Foo();
  EmailMessage.To = emailAddress;
}
else
{
  emailAddress = Bar();
  // Do something different with emailAddress;
}

// No more usages of emailAddress

2: "Improved" example:

This is better because the variables are defined where they are used so there is no need to go looking around for other usages.

if (//condition)
{
  var emailAddress = Foo();
  EmailMessage.To = emailAddress;
}
else
{
  var emailAddress = Bar();
  // Do something different with emailAddress;
}

3: "Even better" example:

I think this is the best it can get because the method names will help with understanding the code.

if (//condition)
{
  ExplanatoryName1(//pass anything in that is required);
}
else
{
  ExplanatoryName2(//pass anything in that is required);
}

4: If you need a return from the methods:

var methodReturn = condition ? ExplanatoryName1(args) : ExplanatoryName2(args);

Answer 2

The scope of local variables should always be as minimal as possible. So, I would declare the variable precisely there where it is used even if I have to re-declare it in multiple places within the same method. As a matter of fact, I would declare it precisely where it is used even if there was no if condition. Many people do not know it, but you can start a new scope with curly brackets anywhere you want within a method, even if you do not have a control statement to put it under. For example:

//some code

//comment explaining what the following does:
{
    string emailAddress = Foo();
    EmailMessage.To = emailAddress;
}

//more code

//comment explaining what the following does:
{
    string emailAddress = Boo();
    SomeOtherEmailMessage.From = emailAddress;
}

//yet more code

One more note: I see that when your colleague declared the email address variable he assigned string.Empty to it, and then further down he re-assigned a meaningful value to it prior to using it. Some people believe that when declaring a local variable you should always initialize it with some initial value, and they follow this rule in an almost superstitious fashion. This is wrong. It may have been advisable back in the times when all we had was some primitive and crude C compilers, but not anymore. Modern compilers of C, C++, C# and Java are quite good at warning you if a variable might be read before it has been initialized. So, by pre-initializing the variable at declaration time with a value which is by definition meaningless, (since a meaningful value is not yet known at that time,) you are circumventing the safety checks of your compiler, and you are actually opening up the possibility of error: if you forget to assign a proper value to your variable further down before you actually make use of it, the compiler will not warn you, because as far as the compiler knows, the variable has already been initialized at declaration time.

Furthermore, even if the compiler was incapable of warning about uninitialized values, you would still have to wonder which outcome is better for a program with an uninitialized variable bug: to fail with a null pointer exception the first time it is used, or to appear to work but never send any messages to anyone.

In any case, you should not really have to worry about the above note, because if you follow the rule which says that the scope of local variables should always be as minimal as possible, you will always be initializing your local variables with meaningful values precisely at the moment that you are declaring them.

Answer 3

I think it sort of depends on the context. Like you said, for smaller methods where there is only a few lines then declaring at the top of the method will be neither here nor there as the reader can easily see what the entire method is doing.

However my preference is to declare the variables as close to their first usage as possible. A few reasons.

1. I believe a variable should be used for one thing only. By declaring them close to their usage then you are implicitly stating this is where the variable is being use.
2. Easier to see when that variable is used in the code. If you method is a few lines long then you can easily see where a variable is first used.
3. It might help with identifying refactoring of code. For example by adding the variables inside the scope of the condition above you might come to decide that actually the condition could be done elsewhere.

Answer 4

Read Effective Java, Second Edition, Item 45: Minimize the scope of local variables. It has a good overview on the topic. (Google for "minimize the scope of local variables", it's on Google Books too.)

Answer 5

In C# the variables have the scope of their code block, compared to Javascript for example where variables have the scope of the entire function. In Javascript it makes sense to declare all variable at the top of the function, because that is their actual scope.

You should minise the scope of variables, so that you can't use them in the wrong part of the code by accident.

Example:

void DoSomething(bool write) {

  string source = String.Empty;
  string destination = String.Empty;
  string data = String.Empty;

  if (write) {
    destination = "some other thing";
    WriteTo(destination, data);
  } else {
    source = "something";
    data = ReadFrom(destination); // oops, used the wrong variable
  }

}

Answer 6

Before I read Clean Code by Robert C. Martin, I would've said that you should go with your colleague's option (group all the variables at the top) because it looks better/cleaner and it makes more sense (the last reason comes, without a doubt, from a couple of years of C/C++). Nowadays, I pay attention to variable scope and clean code practices; so the version which I would use is:

public void SendEmail()
{
    if (//condition)
    {
        string emailAddress = GetEmailAddress();       
        EmailMessage.To = emailAddress;

        string managerEmailAddress = GetManagerEmailAddress();
        EmailMessage.CC = managerEmailAddress;
    }
}

which, depending on the rest of the code, can be further improved ("extract 'till you drop").

Still, I can think of a possible problem on this subject, when you apply refactoring before seeing all the usages. Consider the original code:

public void SendEmail()
{
    string emailAddress = string.Empty;
    string managerEmailAddress = string.Empty;

    if (//condition)
    {
        //do something
        //Retrieve emailAddress and managerEmailAddress
        EmailMessage.To = emailAddress;
        EmailMessage.CC = managerEmailAddress;
    }

    EmailMessage.BCC = emailAddress;
}

...and the refactored one:

public void SendEmail()
    {
        if (//condition)
        {
            string emailAddress = GetEmailAddress();       
            EmailMessage.To = emailAddress;

            string managerEmailAddress = GetManagerEmailAddress();
            EmailMessage.CC = managerEmailAddress;
        }

        EmailMessage.BCC = emailAddress;  //now what?
    }

So, when you refactor the code, you need to keep an eye to the scope.