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.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Sean Chambers

I am a Senior software developer from Palm Coast, Florida. An advocate of Domain Driven Design, Behavior Driven Development, creator of FluentMigrator and community activist. I am married to my beautiful wife Erin and am the proud father of two wonderful children. I currently reside at ACI, a local insurance industry/mortgage software company that excels in creating solutions using Agile methodologies.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://craniac.net Steve Crane

    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.

  • http://weblogs.asp.net/gunnarpeipman Gunnar Peipman

    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.

  • http://www.lostechies.com/members/schambers/default.aspx schambers

    @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.

  • http://weblogs.asp.net/gunnarpeipman Gunnar Peipman

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