| Essay 31 | Know When to Refactor |
Another smell of complexity arises when we think too far ahead in our code. There’s a price to pay for being too cute or too cerebral about the actual thing we’re trying to build. A classic case is implementing a design pattern too early.
Don’t get me wrong, design patterns are wonderful things. When a common programming approach happens over and over again, we get excited. We’ve all experienced that sense that our code could be doing something greater than just solving the concrete task at hand.
When we’ve had this feeling a few times and successfully refactored our code into more abstract patterns, it’s easy to feel invincible. We work like a crime dog, sniffing out any small sign or clue, any hint, that another abstraction lives above our straightforward piece of code.
But very quickly, our sixth sense can come back to bite us where it really hurts. Most of us have heard, or experienced, the horror stories of “architecting yourself into a corner.” It’s where we’ve taken an abstract approach to solving a problem way too far.
The Danger of Refactoring Too Soon
For example, suppose you’re working on an intranet for Burgeoning Web Company. They’re small, and they have only two departments: IT and sales. The executives at Burgeoning Web Company want the ability to calculate anticipated bonuses for their employees based on a bunch of employee parameters. But each department wants to base bonuses on different measurements.
The IT department only wants to give bonuses to employees as a percentage of their current salary and only to those who have stuck around for five years. The sales department wants to give everyone a $1,000 base bonus, plus a standard $500 incremental bonus for each year they’ve worked at Burgeoning Web Company. After all, the company is burgeoning, and the execs are generous folks.
You start coding. You’ve built an Employee class that will contain all the information you need to calculate an employee’s bonus. You then write a simple function, which, for now, contains one simple conditional statement to return a given employee’s anticipated bonus.
| | public decimal GetBonusForEmployee(Employee employee) |
| | { |
| | if (employee.department == Departments.IT) |
| | { |
| | // Calculate bonus the "IT" way |
| | if (employee.Years >= 5) |
| | { |
| | return .1 * employee.Salary; |
| | } |
| | |
| | return 0; |
| | } |
| | else |
| | { |
| | // Calculate bonus the "Sales" way |
| | return 1000 + 500 * employee.Years; |
| | } |
| | } |
You write a little tool to load all employees into a collection of Employee objects and then apply the previous method to each. Your work is done. Beer time.
But your mind begins to think about the other possibilities that lie ahead when Burgeoning Web Company really begins to burgeon. What do you do when the third or fourth department comes along? Swap the conditional logic for a switch statement! But why wait? Let’s anticipate it now so you’re ready for the future:
| | public decimal getBonusForEmployee(Employee employee) |
| | { |
| | switch(employee.department) |
| | { |
| | case Departments.IT: |
| | |
| | // Calculate bonus the "IT" way |
| | if (employee.Years >= 5) |
| | { |
| | return .1 * employee.Salary; |
| | } |
| | |
| | return 0; |
| | |
| | case Departments.SALES: |
| | |
| | //Calculate bonus the "Sales" way |
| | return 1000 + 500 * employee.Years; |
| | } |
| | } |
Beautifully done! The switch statement is a safe anticipatory move. It explicitly identifies the sales department instead of relegating it to the else statement. When marketing comes along, you know just where it fits.
This small refactoring makes sense. Your code is now more explicit, and it’s easier to scan. Another developer could come in and pick it up right away.
Sensing your higher calling, you decide to do more. What happens when two departments become....ten? In a few months, there could be new departments springing up, such as legal, production, accounting, and the janitorial staff. The switch statement will eventually get unwieldy. It will be tainted with complex calculations that have no business lying there, exposed so nakedly at the surface of the bonus calculation method.
You rifle through your favorite design patterns book (I highly recommend Joshua Kerievsky’s Refactoring to Patterns [Ker04]) and—voilà—the Strategy pattern! Move all those one-off bonus calculations into individual strategy classes (e.g., ITBonusCalculationStrategy and SalesBonusCalculationStrategy) that each implement a Bonus Calculation Strategy interface (IBonusCalculationStrategy). The interface will require each implementing class to define a CalculateBonus method.
Once that’s done, you modify the Employee class to contain a concrete strategy instance and create one new public method that will return an employee’s bonus.
With the Strategy pattern, you can now remove the getBonusForEmployee method altogether. The calculation of an employee’s bonus can live in the class itself. So, all those nasty algorithms lie elegantly in the soft cushiony pillows of individual implementations of the IBonusCalculationStrategy interface.
Since you’ve gone this far, you decide to embellish your code. You abstract the creation of employees into a Factory pattern. This way, you can create department-specific employee creator classes to assign a corresponding bonus strategy for an employee.
You’ve completely removed the conditional switch on departments (it’s taken care of in the employee creator classes) and the nasty calculation logic (it’s buried in department-specific strategy classes). Wonderful!
Once department 15 comes along, this architecture will be a sight to see.
Weeks and months go by. The winter hits, and times are tough for Burgeoning Web Company. Still no new departments. Meanwhile, the bonus logic has changed. You go back in, stepping through code and wondering what happened to your once simple logic. Ah yes. It’s been strategized and factoryized.
Another couple months go by. Burgeoning Web Company calls and says they’ve fired the sales team, and it’ll just be the CEO working the phone with his team of developers. Wanting to keep developer morale, the CEO still wants to offer bonuses but now based solely on seniority.
It’s time to shed a tear. You’ve placed all your bets on the department-specific bonus rules. It was, by all accounts, a safe bet one year ago. You’ve built the walls, ladders, and slides to account for every possible department bonus for the next one hundred years. The entire refactoring has gone to waste. It’s not just over-architected clutter; it’s spoiled clutter. You unearth your strategy classes, remove the factory methods, and submissively decide that a potentially nasty but quite all right conditional statement will do just fine for now.
Patterns are wonderful concepts. However, they should be implemented with the utmost caution. Anticipating logic in the future, more often than not, will lead to unintended complexity.
If there is a golden rule, it’s that an application shouldn’t be forced into a well-documented design pattern or set of patterns. Rather, a design pattern (or set of them) should be implemented as fully as needed to fit the desired tasks of the application and the most likely scenarios for the near future.
When you study a design pattern, read it as a general approach to solving a particular problem, not as a strict, rigid solution to a problem. Patterns all have pros and cons. While patterns make some tasks more elegant to perform, you always lose something else. Since most of today’s web applications are constantly changing based on new customer or client requirements, finding the “perfect” set of patterns from the get-go is more dream than reality.
Does this mean we shouldn’t anticipate for change at all? No. Problems manifest when we don’t pay any attention to where our architecture is headed.
The Headache of Unmanageable Legacy Code
Examine any codebase whose authors decided not to make simple refactorings when they were necessary. The problems reveal themselves quickly. Variables are scoped at the wrong level or, even worse, accessible globally with some bizarre naming convention to ensure they’ll always be unique. Conditional logic reads more like the terms on a Terms of Use page: a bunch of unrelated truths stitched together with ands and ors that have collectively lost all meaning to the next unlucky soul who has to modify it.
We find this often in legacy code that’s been handed down from generation to generation of developers that came into it without much passion and came out of it with even less. Method signatures become unruly, and method calls look like the code itself isn’t sure what it’s doing:
| | calculateBonusesForTeam(.02, 155000, null, 0, 0, null, |
| | new Employee(), null, null, false, true); |
Over time, unconsidered refactorings get expensive. Maintenance becomes asymptomatically slower. Forget big changes; even small changes, the ones we always take for granted, might collapse a codebase that’s long since forgotten any basic set of good habits.
Let’s get back to the Burgeoning Web Company story. At some point, refactoring the bonus calculations into a Strategy pattern might have made sense. Moving employee creation methods into a factory class might have been useful. It just wasn’t at the time.
Over-architect too early in the development life cycle, and we’re left with a hole waiting to be filled. Under-architect, and we’re left without any option or motivation to evolve our software any further.
“The hole and the patch should be commensurate.” —Thomas Jefferson to James Madison
Perhaps Jefferson was channeling his conversation with Benjamin Franklin a few years earlier.
Anticipate, but anticipate cautiously. Whether it’s just a small change or a large pattern shift, know what you’re gaining and losing each time you decide to refactor.