When Not to Refactor

for RuBoard

There are situations when refactoring makes less sense than it normally does. One such situation is when refactoring comes at the expense of performance. Contrary to Fowler's otherwise masterful treatment of the subject, I do not believe that refactoring code such that it performs poorly is a good idea, regardless of the frequency with which it is called. In Refactoring, Fowler ^[13] provides an example in which a temporary variable is eliminated by calling a function repeatedly rather than caching its result. So, rather than having a single call to the function, with its result stored in a variable that is then referenced by the remainder of the method, we have multiple calls. Each time the function's result is needed, the function is called. This, despite the fact that the function is deterministic in nature. Its result never changes between calls. Fowler ^[14] refers to this refactoring as the Replace Temp with Query technique.

^[13] Ibid. Page 27.

^[14] Ibid. Page 120.

This is a poor practice for a number of reasons. First, it leads to performance problems that will likely have to be cleaned up later. Although performance tuning is technically not refactoring, this type of development defeats the whole purpose of evolving your code (of which refactoring is only a part), which is to improve it gradually over time. A change is, by definition, not an "improvement" if it changes performance for the worse . Being easy to read and maintain is only part of the story. In Fowler's example, if the function being called takes a significant amount of time to return, you'll have to "unfactor" your refactoring in order to attain reasonable performance. Second, even if the code does not require an inordinate amount of time to execute, or is not called often, it can still negatively affect performance. Big performance problems are often aggregations of little ones. That is, no single call in a chain of execution may be prohibitively expensive, but, taken together, the entire chain can simply take too long to execute. The little foxes spoil the vines. Coding like this will leave you scratching your head for hours, trying to find out where you went wrong and what you can do about it. Nothing will jump off the screen as the obvious culprit of your performance problems. Instead, everyone's to blame and an inefficient style of coding is the real culprit.

A case in point here is screen updating. Often, no single line of code is to blame for subpar screen updates. Instead, the problem is usually one of approach: Individually, many of the lines being executed simply aren't efficiently coded. To fix them, you'll get to do a fair amount of rewriting. It's far better to code with efficiency in mind in the first place, than to have to work and rework code just to get acceptable performance.

Fowler's ^[15] contention here is that because temporary variables are useful only within the routine that contains them, they lead to overly long and complex routines. This is a good point, but it's far from compelling. The simple answer is to avoid writing such long routines in the first place and to break up inordinately long ones when you find them. The use or nonuse of temporary variables is a separate issue. Fowler ^[16] also contends that you cannot know before you profile the code what effect Replace Temp with Query will have on performance. I disagree with this as well. If you have some idea of how long a function should take to return, you can have a fair idea of the ramifications on performance of calling the function unnecessarily. It may not be scientific or exact, but you can have an empirical estimate of the performance cost of such an inefficiency, and you can have it without firing up a code profiler.

^[15] Ibid. Page 26.

^[16] Ibid. Page 32.

One concession I would make on the issue is that constructing method functions to return the result of a computation is generally more useful than storing the results of the computation in a variable, especially to other methods. Adding these functions to the interface of the class to make them available to other methods is trivial, and can prove to be quite useful. This said, I still would temper my enthusiasm for this with a good dose of pragmatism . Coding an interface inefficiently because it may be used in the future makes little sense when refactoring is the order of the day anyway. ^[17] You can always go back and extract a method if the need actually arises. Until then, don't refactor when it costs you tangible application performance and yields little in the way of making the code easier to follow.

^[17] Beck, Kent. Extreme Programming Explained: Embrace Change . Reading, MA: Addison- Wesley, 2000. Page 57.

for RuBoard