In this article I’ll be walking through an example of how to refactor the Long Method code smell.
Code Smell: Long Method
Definition: A method has too many lines of code, making it hard to understand.
Solution:
- Extract duplicate code into a new method
- Extract code into smaller methods
Long Method code smell example
Here’s an example of the Long Method code smell (source: GildedRose Refactoring Kata). This method is 75 lines long, and is full of code smells. However, when refactoring we need to focus on one step at a time. In this case I’ll be focusing on refactoring this Long Method code smell.
public void UpdateQuality()
{
for (var i = 0; i < Items.Count; i++)
{
if (Items[i].Name != "Aged Brie" && Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
{
if (Items[i].Quality > 0)
{
if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
{
Items[i].Quality = Items[i].Quality - 1;
}
}
}
else
{
if (Items[i].Quality < 50)
{
Items[i].Quality = Items[i].Quality + 1;
if (Items[i].Name == "Backstage passes to a TAFKAL80ETC concert")
{
if (Items[i].SellIn < 11)
{
if (Items[i].Quality < 50)
{
Items[i].Quality = Items[i].Quality + 1;
}
}
if (Items[i].SellIn < 6)
{
if (Items[i].Quality < 50)
{
Items[i].Quality = Items[i].Quality + 1;
}
}
}
}
}
if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
{
Items[i].SellIn = Items[i].SellIn - 1;
}
if (Items[i].SellIn < 0)
{
if (Items[i].Name != "Aged Brie")
{
if (Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
{
if (Items[i].Quality > 0)
{
if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
{
Items[i].Quality = Items[i].Quality - 1;
}
}
}
else
{
Items[i].Quality = Items[i].Quality - Items[i].Quality;
}
}
else
{
if (Items[i].Quality < 50)
{
Items[i].Quality = Items[i].Quality + 1;
}
}
}
}
}
Code language: C# (cs)Before we begin
Refactoring rule #1: Always make sure you have tests covering the code you’re about to refactor. Run the tests after each small step.
Extract duplicate code into a new method DecrementQualityForNormalItems(int i)
I have to zoom out to see this entire method. After doing that, I can see that there is duplicate code.
if (Items[i].Quality > 0)
{
if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
{
Items[i].Quality = Items[i].Quality - 1;
}
}
Code language: C# (cs)I can reduce the length of the method by putting duplicate code into a new method and calling it. This is known as the Extract Method refactoring.
1. Create a new method called void DecrementQualityForNormalItems(int i).
2. Copy the duplicate code into DecrementQualityForNormalItems(int i).
3. Replace the duplicate code with a call to DecrementQualityForNormalItems(int i).
Extract duplicate code into a new method IncrementQuality(int i)
I noticed more duplicate code, so I’ll apply the Extract Method refactoring again.
if (Items[i].Quality < 50)
{
Items[i].Quality = Items[i].Quality + 1;
}
Code language: C# (cs)1. Create a new method called void IncrementQuality(int i).
2. Copy the duplicate code into IncrementQuality(int i).
3. Replace the duplicate code with a call to IncrementQuality(int i).
4. Remove the outer if Quality < 50, and replace it with a call to IncrementQuality(int i).
This is a little more tricky than the previous refactoring. The tricky part is that the conditional block has additional logic. We can remove the outer if Quality < 50 because doing so has no side effects. It has no side effects because IncrementQuality() also has the if Quality < 50 condition. Therefore, there is really no need for this outer if.
Extract code into new method UpdateQualityForItemsThatAgeWell(int i)
The next step is to look for blocks of code that go together and apply the Extract Method refactoring. Besides making the method shorter, by having to give a descriptive name to the new method, it makes the code easier to understand.
The following if block can be extracted to a method to improve readability:
IncrementQuality(i);
if (Items[i].Name == "Backstage passes to a TAFKAL80ETC concert")
{
if (Items[i].SellIn < 11)
{
IncrementQuality(i);
}
if (Items[i].SellIn < 6)
{
IncrementQuality(i);
}
}
Code language: C# (cs)1. Create a new method called UpdateQualityForItemsThatAgeWell(int i).
2. Copy the code over to UpdateQualityForItemsThatAgeWell(int i).
3. Replace the code with a call to UpdateQualityForItemsThatAgeWell(int i).
Extract code into new method UpdateQualityForExpiredItems(int i)
Again, I’ll look for code that goes together and use the Extract Method refactoring. The usual targets for this are if blocks and loops, such as the following if block:
if (Items[i].Name != "Aged Brie")
{
if (Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
{
DecrementQualityForNormalItems(i);
}
else
{
Items[i].Quality = Items[i].Quality - Items[i].Quality;
}
}
else
{
IncrementQuality(i);
}
Code language: C# (cs)1. Create a new method called UpdateQualityForExpiredItems(int i).
2. Copy the code over to this new method.
3. Replace the code with a call to UpdateQualityForExpiredItems(int i).
End result
The method is now much shorter and easier to understand.
public void UpdateQuality()
{
for (var i = 0; i < Items.Count; i++)
{
if (Items[i].Name != "Aged Brie" && Items[i].Name != "Backstage passes to a TAFKAL80ETC concert")
{
DecrementQualityForNormalItems(i);
}
else
{
UpdateQualityForItemsThatAgeWell(i);
}
if (Items[i].Name != "Sulfuras, Hand of Ragnaros")
{
Items[i].SellIn = Items[i].SellIn - 1;
}
if (Items[i].SellIn < 0)
{
UpdateQualityForExpiredItems(i);
}
}
}
Code language: C# (cs)