Refactoring: Extract Method example in C#

Many years ago I read the book Refactoring - Improving the design of existing code by Martin Fowler and found that it contained lots of interesting thoughts about code readability.

Instead of just doing a reread of it, I thought it might be interesting to write about the refactors that I've had the most use of during my time as a (mostly web) developer.

The examples in the book are written i Java, I will copy and translate them to C#.

I begin the series with the Extract Method refactoring.

Extract Method

You have a code fragment that can be grouped together.

Turn the fragment into a method whose name explains the purpose of the method.

The code before

The example is a short method that prints out how much a customer owes.

1:  public void PrintOwing()  
2:  {  
3:    Decimal outstanding = 0;  
4:    
5:    // print banner  
6:    Console.Out.WriteLine("*************************");  
7:    Console.Out.WriteLine("***** Customer Owes *****");  
8:    Console.Out.WriteLine("*************************");  
9:    
10:    // calculate outstanding  
11:    foreach (var order in _orders)  
12:    {  
13:      outstanding += order.GetAmount();  
14:    }  
15:    
16:    // print details  
17:    Console.Out.WriteLine("name: " + _name);  
18:    Console.Out.WriteLine("amount: " + outstanding);  
19:  }  

The code after

The three sections in PrintOwing has been moved out to three separate methods, which makes the PrintOwing method really short and you get the big picture of what it does real easy.

1:  public void PrintOwing()  
2:  {  
3:    PrintBanner();  
4:    var outstanding = GetOutstanding();  
5:    PrintDetails(outstanding);  
6:  }  
7:    
8:  private void PrintBanner()  
9:  {  
10:    Console.Out.WriteLine("*************************");  
11:    Console.Out.WriteLine("***** Customer Owes *****");  
12:    Console.Out.WriteLine("*************************");  
13:  }  
14:    
15:  private decimal GetOutstanding()  
16:  {  
17:    Decimal outstanding = 0;  
18:    foreach (var order in _orders)  
19:    {  
20:      outstanding += order.GetAmount();  
21:    }  
22:    return outstanding;  
23:  }  
24:    
25:  private void PrintDetails(decimal outstanding)  
26:  {  
27:    Console.Out.WriteLine("name: " + _name);  
28:    Console.Out.WriteLine("amount: " + outstanding);  
29:  }  

Motivation

The big wins with the extract method refactoring is that the code is divided into fine grained chunks that increases the chance of code reuse, and that the code itself can be read more like a series of comments.

How to Refactor

Read more about how to do this refactor step-by-step and more here.

The source code

https://github.com/carlbjorknas/RefactoringExamplesInCSharp.git

Personal thoughts

Replace foreach with Linq

I really like to Linq-ify code when it makes the code shorter and better expresses what it does. Like replacing a foreach loop with Linqs Sum method.

I prefer this

1:  private decimal GetOutstanding()  
2:  {  
3:    return _orders.Sum(order => order.GetAmount());  
4:  }  

instead of this

1:  private decimal GetOutstanding()  
2:  {  
3:    Decimal outstanding = 0;  
4:    foreach (var order in _orders)  
5:    {  
6:      outstanding += order.GetAmount();  
7:    }  
8:    return outstanding;  
9:  }  

because I think Sum shows more clearly than the foreach loop what we're after.

Thoughts that popped up during writing

The PrintOwing method was short to begin with. Did the three extract method operations really make it better? I've performed the extract method on much longer methods, where I clearly saw the advantage, but here I don't find it quite as clear.

One thing that happens when you extract code to smaller methods is that its makes the code easier to reuse. Suppose I want to print the same banner from another method, after the refactoring I can just call the PrintBanner method instead of copy-pasting the code.

So, what's the problem with copy-paste coding? Well, suppose you need to print the same banner in five different methods. If you haven't extracted the banner printing code to a separate method the same code will exist at five places. What happens when your customer wants you to change the banner layout, maybe change the "*" to a "#"? Will you find every copy of the banner printing code? With five different copies of it, I think the risk is high that some of the copies will be forgotten to be changed, and voila, you've created a bug!

My guess is that although PrintOwing was short from the start, it is possible to understand the big picture about what it does faster when it is even shorter. Compare these two really hypothetical eye movement scanning examples when reading the code to get the big picture of what PrintOwing does.

In the example above, you have to read a lot of code to understand the big picture of what the code does. That's bad, but on the other side, you get a good bit of insight into the details. You can see that the eyes make big jumps up and down when looking for a variable and what value it has been assigned to, and how it has been used. Details are mixed with the bigger picture.

In this second example the eyes have to scan a lot less code and don't have to do big jumps to find out where the variable gets its value from.

Maybe you would scan the code in the first example a lot different than I've drawn. Maybe you scan for comments, read them and skip the code. Well, I read comments as well, but my experience is that way too often the comment related to the code does not really match the code. The comment says one thing, and the code does something like it but not quite, or maybe even something waaay off.

In this example, the comments are replaced with the names of the methods the code is extracted to, so the problem can still exist, if the method is given a bad name it won't match the code it contains.