Overview
Extract method is probably the refactoring I use most frequently. It is covered in detail in The Refactoring Book, so I’ll leave out many of the details here that the book covers. I strongly encourage anyone interested in refactoring to read this book and keep it close by as a reference. The intent of this post and of this series of daily refactorings is to introduce a set of commonly used refactorings and to spark discussion about concrete real-world experience with them.
I want to get people thinking about the importance of refactoring and how it impacts how we think about upfront design vs just-in-time design and the ways in which refactoring can help us avoid over engineering.
What is Extract Method?
A refactoring that moves code out of a method by creating a new method and delegating to it.
To illustrate, let’s start with some code that could benefit from applying extract method:
public BigDecimal calculateTotal(List<item> items) { // calculate sub total BigDecimal subTotal = new BigDecimal("0.00"); for (Item i : items) { subTotal.add(i.getAmount()); } // calculate tax BigDecimal taxRate = TaxRateHelper.getTaxRateForState(_user.getState()); BigDecimal taxAmount = subTotal.multiply(taxRate); BigDecimal total = taxRate + taxAmount; total.setScale(2, BigDecimal.ROUND_HALF_EVEN); return total; }
This code isn’t terrible by any stretch but it could be cleaned up by applying extract method. It’s easy to see that more than one thing is being done in this method. It is calculating a sub-total, tax amount, and total. Let’s extract these out as separate methods:
public BigDecimal calculateTotal(List<item> items) { BigDecimal subTotal = calculateSubTotal(items); BigDecimal taxAmount = calculateTaxAmount(subTotal); BigDecimal total = taxRate + taxAmount; total.setScale(2, BigDecimal.ROUND_HALF_EVEN); return total; } public BigDecimal calculateSubTotal(List<item> items) { BigDecimal subTotal = new BigDecimal("0.00"); for (Item i : items) { subTotal.add(i.getAmount()); } return subTotal; } public BigDecimal calculateTaxAmount(BigDecimal amount) { BigDecimal taxRate = TaxRateHelper.getTaxRateForState(_customer.getState()); BigDecimal taxAmount = subTotal.multiply(taxRate); return taxAmount; }
What impact did this refactoring have? What were some of the benefits?
- The original calculateTotal method is more cohesive and sticks to doing one thing.
- The comments that described what the different sections of code were doing are no longer required because the method name clearly documents what the code does.
- Each of the methods calculateTaxAmount and calculateSubTotal are no longer tightly coupled to calculateTotal and can be called independently.
Of all the benefits provided perhaps the one that I think has the most impact is that extract method increases cohesiveness and decreases coupling. The benefits of having a loosely coupled, highly cohesive system are well-worth the effort it takes to perform extract method, especially considering most modern development tools (such as ReSharper, IntelliJ, and Eclipse) support this refactoring.
Can you see any other refactorings that this code could benefit from?
5 comments ↓
Those methods you extracted should be made private, otherwise they are just polluting your exposed api with worker methods that shouldn’t be seen externally.
In the tax amount method, where is _customer coming from, is this a global variable, doesn’t seem like it should be, there should be a general purpose method for retrieving the tax rate somewhere else.
Hi Chris! Thanks for the comments.
The new methods are now part of the API. What’s the harm in exposing them? Now when another developer comes along and needs to calculate a sub-total it’s already there. Any new code that uses them should be developed using TDD so I wouldn’t be concerned about test coverage.
I can think of cases where I would chose to make methods private, but I typically default to public unless I have a good reason for restricting access.
However, by *exposed* API you may be referring to a published API which is a separate topic altogether. I agree that careful consideration should be made when changing a published API.
You’re right that _customer is implied and not clearly defined (oops). For this example just consider it to be a field on the class.
It just seems to me that the subtotal and tax methods are likely to only be used inside this class. Defaulting everything to public pollutes intellisense. I agree with making methods public if at the time of creation they are designed to be used outside of their current class, but if they’re only intended for internal use at the time I prefer to make everything private. It’s access modifier can be changed later if need be.
Pretty sure there would be many opinions on how these should get set
There are good arguments for both sides. I think the pollution of intellisense is minimized when classes are highly cohesive.
I’ll show later how extracted methods can sometimes exhibit object envy and how move method can cure this.
For example, although I didn’t create the above example to demonstrate this, List could be represented by its own class such as ItemCollection. With this, calculateSubTotal clearly exhibits object envy (it envies the information encapsulated within ItemCollection). Moving calculateSubTotal to ItemCollection solves this problem.
Now it makes perfect sense to have a public method called calculateSubTotal on ItemCollection.
I find that refactorings work together in concert and form a series of “moves” that bring the system into a better state. Without seeing the overall strategy to these moves, a single move on it’s own may not make as much sense.
I was going to just make a comment saying that after some more thought I came to the same conclusion as you outlined in your last comment
Leave a Comment