Local Variables Are Evil (or Just Irritating)

Every now and then, I reread parts of Refactoring by Martin Fowler. The main reason is of course to look for refactoring ideas for DevExpress' Refactor! Pro product, but I also like to take a particular refactoring in there, tease it out, and really understand it.

Such a refactoring is Replace Temp with Query (120). The preamble to the refactoring goes like this:

You are using a temporary variable to hold the result of an expression.

Extract the expression into a method. Replace all references to the temp with the expression. The new method can then be used in other methods.

The motivation for the refactoring is very interesting because it essentially says "don't use local variables":

The problem with temps is that they are temporary and local. Because they can be seen only in the context of the method in which they are used, temps tend to encourage longer methods, because that's the only way you can reach the temp. By replacing the temp with a query method, any method in the class can get at the information. That helps a lot in coming up with cleaner code for the class.

There's actually something more to consider about local variables or temps and that is using them can make a method harder to simplify through repeated uses of Extract Method (110). If a local is read-only (say a parameter passed into the method) and the code-to-be-extracted makes use of it, then the local will become a parameter to the extracted method.

If however the local is both read and assigned to in the code-to-be-extracted (the variable is said to be mutable), then the new value will have to be returned by the new extracted method (as well as the local becoming a parameter). Not to shabby if there's only one such local, but if there are more, things get hairy quite quickly. (Aside: in Refactor! Pro demos, we like to show this off, since the "hairiness" is magnified by the number of input and output arrows in the preview.)

Let's take an example, and, to be fair, let's take an example I wrote. One of the more popular articles I authored for my previous blog on www.boyet.com is the one where I discuss how to calculate the ISO week number for a given date. I'll repeat the complete code here, but it is instructive to read the whole article.

  public class WeekCalculator {

    private static DateTime GetIsoWeekOne(int year) {
      // get the date for the 4-Jan for this year
      DateTime dt = new DateTime(year, 1, 4);

      // get the ISO day number for this date 1==Monday, 7==Sunday
      int dayNumber = (int)dt.DayOfWeek; // 0==Sunday, 6==Saturday
      if (dayNumber == 0) {
        dayNumber = 7;
      }

      // return the date of the Monday that is less than or equal
      // to this date
      return dt.AddDays(1 - dayNumber);
    }

    public static int GetIsoWeek(DateTime dt) {
      DateTime week1;
      int IsoYear = dt.Year;
      if (dt >= new DateTime(IsoYear, 12, 29)) {
        week1 = GetIsoWeekOne(IsoYear + 1);
        if (dt < week1) {
          week1 = GetIsoWeekOne(IsoYear);
        }
        else {
          IsoYear++;
        }
      }
      else {
        week1 = GetIsoWeekOne(IsoYear);
        if (dt < week1) {
          week1 = GetIsoWeekOne(--IsoYear);
        }
      }

      return (IsoYear * 100) + ((dt - week1).Days / 7 + 1);
    }
  }

If you peruse that, you'll see lots of local variables. In fact, as I look at it now, I can see Extract Method screaming at me from all the comments in the GetIsoWeekOne() method, so let's start there.

Ignoring the year parameter, there are two mutable local variables: dt and dayNumber. Let's first get rid of the latter of those two using Replace Temp with Query. The code for GetIsoWeekOne() now looks like this:

    private static int GetIsoDayNumber(DateTime date) {
      if (date.DayOfWeek == DayOfWeek.Sunday)
        return 7;
      return (int)date.DayOfWeek; 
    }
    private static DateTime GetIsoWeekOne(int year) {
      // get the date for the 4-Jan for this year
      DateTime dt = new DateTime(year, 1, 4);

      // return the date of the Monday that is less than or equal
      // to this date
      return dt.AddDays(1 - GetIsoDayNumber(dt));
    }

That return expression is calling out for some help:

    private static DateTime GetPreviousMonday(DateTime date) {
      return date.AddDays(1 - GetIsoDayNumber(date));
    }
    private static DateTime GetIsoWeekOne(int year) {
      // get the date for the 4-Jan for this year
      DateTime dt = new DateTime(year, 1, 4);
      return GetPreviousMonday(dt);
    }

And now we can get rid of the temp completely. There's an obvious way, but I decided to make it a little cleaner still. Here's the fully refactored code:

    private static int GetIsoDayNumber(DateTime date) {
      if (date.DayOfWeek == DayOfWeek.Sunday)
        return 7;
      return (int)date.DayOfWeek; 
    }
    private static DateTime GetPreviousMonday(DateTime date) {
      return date.AddDays(1 - GetIsoDayNumber(date));
    }
    private static DateTime Get4thOfJanuary(int year) {
      return new DateTime(year, 1, 4);
    }
    private static DateTime GetIsoWeekOne(int year) {
      return GetPreviousMonday(Get4thOfJanuary(year));
    }

The nice thing about this code is that there's lots of opportunities for the .NET JITter to inline the code at run-time.

OK, let's move onto the next method, GetIsoWeek, the public one we call. Here there are three locals (dt, week1, IsoYear), the latter two of which are temps. In fact IsoYear is that most malignant of all temps: it's mutable (check out the IsoYear++ and --IsoYear expressions). Extracting a method out of this is not going to be fun, but I'm inclined to make the effort because the original code is so nasty looking.

In essence, there are three cases we need to look at. The date we're given is:

• less than the date for week one. We calculate the week number based on the previous year.
• greater than or equal to the date for week one of the following year. We calculate the week number based on the next year.
• in between those two values for week one. We calculate the week number based on the date's year.

If we didn't try and do the premature optimizations I was so intent on doing last time, the code would simplify to this by removing the week1 temp:

    public static int GetIsoWeek(DateTime dt) {
      int IsoYear;
      if (dt < GetIsoWeekOne(dt.Year))
        IsoYear = dt.Year - 1;
      else if (dt >= GetIsoWeekOne(dt.Year + 1))
        IsoYear = dt.Year + 1;
      else
        IsoYear = dt.Year;
      return (IsoYear * 100) + ((dt - GetIsoWeekOne(IsoYear)).Days / 7 + 1);
    }

Now we're cooking with gas. Let's extract that return expression.

    private static int CalculateIsoWeek(DateTime date, int isoYear) {
      return (isoYear * 100) + ((date - GetIsoWeekOne(isoYear)).Days / 7 + 1);
    }
    public static int GetIsoWeek(DateTime dt) {
      int IsoYear;
      if (dt < GetIsoWeekOne(dt.Year))
        IsoYear = dt.Year - 1;
      else if (dt >= GetIsoWeekOne(dt.Year + 1))
        IsoYear = dt.Year + 1;
      else
        IsoYear = dt.Year;
      return CalculateIsoWeek(dt, IsoYear);
    }

Heh, now we can get rid of the final temp altogether:

    public static int GetIsoWeek(DateTime date) {
      if (date < GetIsoWeekOne(date.Year))
        return CalculateIsoWeek(date, date.Year - 1);
      if (date >= GetIsoWeekOne(date.Year + 1))
        return CalculateIsoWeek(date, date.Year + 1);
      return CalculateIsoWeek(date, date.Year);
    }

Looks good, but there's some implied repeated calculations of the date of ISO week 1 for various years in there and my performance bump is throbbing. Let's see if we can't do better. The essence of the design is we're trying to place the given date in one of three ranges: less than this year's start of week 1, in between this year's week 1 and next year's week 1, and greater than or equal to the start of next year's week 1. So let's explicitly write it that way and make use of previous calculations of the dates of week 1:

    private static int CalcIsoWeek(DateTime date, DateTime weekOne) {
      return (weekOne.Year * 100) + ((date - weekOne).Days / 7 + 1);
    }
    private static int GetIsoWeekUsingRange(DateTime date, DateTime thisWeekOne, DateTime nextWeekOne) {
      if (date < thisWeekOne)
        return CalcIsoWeek(date, GetIsoWeekOne(date.Year - 1));
      if (date < nextWeekOne)
        return CalcIsoWeek(date, thisWeekOne);
      return CalcIsoWeek(date, nextWeekOne);
    }
    public static int GetIsoWeek(DateTime date) {
      return GetIsoWeekUsingRange(date, GetIsoWeekOne(date.Year), GetIsoWeekOne(date.Year + 1));
    }

With this new code, we only calculate two week 1 dates every time, and the third only if needed.

The whole code now looks like this:

  public class WeekCalculator {
    private static int GetIsoDayNumber(DateTime date) {
      if (date.DayOfWeek == DayOfWeek.Sunday)
        return 7;
      return (int)date.DayOfWeek; 
    }
    private static DateTime GetPreviousMonday(DateTime date) {
      return date.AddDays(1 - GetIsoDayNumber(date));
    }
    private static DateTime Get4thOfJanuary(int year) {
      return new DateTime(year, 1, 4);
    }
    private static DateTime GetIsoWeekOne(int year) {
      return GetPreviousMonday(Get4thOfJanuary(year));
    }
    private static int CalcIsoWeek(DateTime date, DateTime weekOne) {
      return (weekOne.Year * 100) + ((date - weekOne).Days / 7 + 1);
    }
    private static int GetIsoWeekUsingRange(DateTime date, DateTime thisWeekOne, DateTime nextWeekOne) {
      if (date < thisWeekOne)
        return CalcIsoWeek(date, GetIsoWeekOne(date.Year - 1));
      if (date < nextWeekOne)
        return CalcIsoWeek(date, thisWeekOne);
      return CalcIsoWeek(date, nextWeekOne);
    }
    public static int GetIsoWeek(DateTime date) {
      return GetIsoWeekUsingRange(date, GetIsoWeekOne(date.Year), GetIsoWeekOne(date.Year + 1));
    }
  }

Which is a lot clearer, despite there being many more methods. Each method is much smaller and easier to comprehend on its own and, as I've said, present more opportunities for inlining at run-time.

So, from this little anecdotal experiment, I quite haven't shown that local variables are necessarily evil, just that you can clean code up and refactor it quite dramatically when you remove them.