Los Techies : Blogs about software and anything tech!

Refactoring Day 10 : Extract Method


Today we look at the Extract Method refactoring. This is an extremely easy refactoring with several benefits. First, it helps to make code more readable by placing logic behind descriptive method names. This reduces the amount of investigation the next developer needs to do as a method name can describe what a portion of code is doing. This in turn reduces bugs in the code because less assumptions need to be made. Here’s some code before we apply the refactoring:

   1: public class Receipt
   2: {
   3:     private IList<decimal> Discounts { get; set; }
   4:     private IList<decimal> ItemTotals { get; set; }
   5:  
   6:     public decimal CalculateGrandTotal()
   7:     {
   8:         decimal subTotal = 0m;
   9:         foreach (decimal itemTotal in ItemTotals)
  10:             subTotal += itemTotal;
  11:  
  12:         if (Discounts.Count > 0)
  13:         {
  14:             foreach (decimal discount in Discounts)
  15:                 subTotal -= discount;
  16:         }
  17:  
  18:         decimal tax = subTotal * 0.065m;
  19:  
  20:         subTotal += tax;
  21:  
  22:         return subTotal;
  23:     }
  24: }

You can see that the CalculateGrandTotal method is actually doing three different things here. It’s calculating the subtotal, applying any discounts and then calculating the tax for the receipt. Instead of making a developer look through that whole method to determine what each thing is doing, it would save time and readability to seperate those distinct tasks into their own methods like so:

   1: public class Receipt
   2: {
   3:     private IList<decimal> Discounts { get; set; }
   4:     private IList<decimal> ItemTotals { get; set; }
   5:  
   6:     public decimal CalculateGrandTotal()
   7:     {
   8:         decimal subTotal = CalculateSubTotal();
   9:  
  10:         subTotal = CalculateDiscounts(subTotal);
  11:  
  12:         subTotal = CalculateTax(subTotal);
  13:  
  14:         return subTotal;
  15:     }
  16:  
  17:     private decimal CalculateTax(decimal subTotal)
  18:     {
  19:         decimal tax = subTotal * 0.065m;
  20:  
  21:         subTotal += tax;
  22:         return subTotal;
  23:     }
  24:  
  25:     private decimal CalculateDiscounts(decimal subTotal)
  26:     {
  27:         if (Discounts.Count > 0)
  28:         {
  29:             foreach (decimal discount in Discounts)
  30:                 subTotal -= discount;
  31:         }
  32:         return subTotal;
  33:     }
  34:  
  35:     private decimal CalculateSubTotal()
  36:     {
  37:         decimal subTotal = 0m;
  38:         foreach (decimal itemTotal in ItemTotals)
  39:             subTotal += itemTotal;
  40:         return subTotal;
  41:     }
  42: }

This refactoring comes from Martin Fowler and can be found here

This is part of the 31 Days of Refactoring series. For a full list of Refactorings please see the original introductory post.

Kick It on DotNetKicks.com
Posted Aug 10 2009, 08:30 AM by schambers

Comments

Steve Crane wrote re: Refactoring Day 10 : Extract Method
on 08-10-2009 10:44 AM

It might have been clearer to have the refactored methods simply return the calculated value and call them as

   subTotal -= CalculateDiscounts(subTotal);

   subTotal += CalculateTax(subTotal);

or to have called them CalculateAndDeductDiscounts() and CalculateAndAddTax() if they are doing the addition and deduction too.

Gunnar Peipman wrote re: Refactoring Day 10 : Extract Method
on 08-10-2009 3:57 PM

I can see no point of this if

private decimal CalculateDiscounts(decimal subTotal)

{

   if (Discounts.Count > 0)

   {

       foreach (decimal discount in Discounts)

           subTotal -= discount;

   }

}

But i can see potential danger if Discounts is null. So better write it this way:

private decimal CalculateDiscounts(decimal subTotal)

{

   if (Discounts.Count == null)

       return 0;

   foreach (decimal discount in Discounts)

       subTotal -= discount;

}

Also make these methods at least as protected because one may also want to unit test them.

schambers wrote re: Refactoring Day 10 : Extract Method
on 08-10-2009 4:07 PM

@Gunnar

Good point on checking for Discounts.Count > 0 and changing that to return 0 instead.

As far as setting the methods to protected for the reason of unit testing, I'm not sure I agree with this. A unit test for this class would excercise the public method CalculateGrandTotal with specific inputs. Granted, Discounts would have to be initialized via a ctor or something along those lines to get different Discounts in there. I digress however, my point is you should not be testing those individual methods, but instead should be treating the public method as a black box. I don't care how the class accomplishes it's work, just as long as all code paths are executed for proper coverage.

Testing private methods is a code smell IMO. The whole reason for having them private is because the class knows how to accomplish it's work. You would simply have several different scenarios for testing CalculateGrandTotal to excercise all paths.

Reflective Perspective - Chris Alcock » The Morning Brew #409 wrote Reflective Perspective - Chris Alcock &raquo; The Morning Brew #409
on 08-11-2009 3:36 AM

Pingback from  Reflective Perspective - Chris Alcock  » The Morning Brew #409

Gunnar Peipman wrote re: Refactoring Day 10 : Extract Method
on 08-11-2009 1:16 PM

I agree about unit tests. I just thought that after meeting with sales department discount calculations are not so easy anymore :P

张荣华 wrote 31天重构指南之十:提取方法
on 09-28-2009 2:03 AM

现在让我们来看提取方法这个重构,这是一个简单却又好处多多的重构,首先,它通过提供有意义的方法名会使代码更具可读性;其次见名知义的方法名减少了维护人员的工作量;最后它更好的可读性减少了对代码的臆断,从而...

31 Days of Refactoring « Vincent Leung's .NET Tech Clips wrote 31 Days of Refactoring &laquo; Vincent Leung&#039;s .NET Tech Clips
on 10-28-2009 9:28 AM

Pingback from  31 Days of Refactoring « Vincent Leung's .NET Tech Clips

PetterLiu wrote 31 Days of Refactoring
on 11-27-2009 4:01 AM

Refactoring Day 1 : Encapsulate Collection Refactoring Day 2 : Move Method Refactoring Day 3 : Pull ...

PetterLiu wrote 31 Days of Refactoring
on 11-27-2009 4:03 AM

Refactoring Day 1 : Encapsulate Collection Refactoring Day 2 : Move Method Refactoring Day 3 : Pull ...

Add a Comment

(required)  
(optional)
(required)  
Remember Me?

Enter the numbers above:
Copyright Los Techies 2008, 2009. All rights reserved.
Powered by Community Server (Commercial Edition), by Telligent Systems