Conditionals Aren’t Evil, Unless You Duplicate Them

Conditionals Aren’t Evil, Unless You Duplicate Them

Wai Lee Chin Feman

August 11, 2013

We Hate If Statements

Many of us have been taught that, in OOP, most of a program’s “if” statements can be replaced by polymorphism. We talk about conditional statements as though they are a code smell, used only by programmers who haven't completely grasped polymorphism. This attitude is not hard to find:

At first, this attitude makes sense. We have all written code with heavily-nested conditionals. This code can be basically impossible to read and change; it also tends to have too much knowledge about the system. By definition, if we don’t use conditionals, these if-nests can’t exist! By using polymorphism over conditionals, we separate responsibilities and spread knowledge throughout the system.

I think that we hate conditionals more than we should. Introducing polymorphism too soon will complicate your code, making it hard to understand and test.

A Simple Example

Let’s zoom in and examine a simple conditional. We have a system where an employee's salary is determined by his or her title. Do we need to refactor this code? Is it telling us to refactor away the conditional?

class Employee
		attr_accessor :title, :name

		def initialize(employee_attributes)
				@title = employee_attributes[:title]
				@name = employee_attributes[:name]
		end

		def hours
				if title == :programmer
						80
				elsif title == :manager
						40
				end
		end
end

class Thank
		def generate(employee_attributes)
				employee = Employee.new(employee_attributes)
				"Thank you #{employee.name} for your #{employee.hours} hours of hard work!"
		end
end

A First Attempt At Refactoring

Here’s one way to avoid the if:

class Employee
		attr_accessor :title, :name

		def initialize(employee_attributes)
				@title = employee_attributes[:title]
				@name = employee_attributes[:name]
		end

		def hours
				{programmer: 80, manager: 40}.fetch(title)
		end
end

This is less expressive; we have simply buried our “if” inside a method call. We have also introduced a data structure and method call which did not previously exist. Yuck.

Polymorphic Version

Let’s try again, this time by using polymorphism:

class Employee
		attr_accessor :title, :name

		def initialize(employee_attributes)
				@title = employee_attributes[:title]
				@name = employee_attributes[:name]
		end
end

class Manager < Employee
		def hours
				40
		end
end

class Programmer < Employee
		def hours
				80
		end
end

class Thank
		def generate(employee_attributes)
				employee = EmployeeFactory.new_employee(employee_attributes)
				"Thank you #{employee.name} for your #{employee.hours} hours of hard work!"
		end
end

class EmployeeFactory
		def self.new_employee(employee_attributes)
				if employee_attributes[:title] == :programmer
						return Programmer.new(employee_attributes)
				elsif employee_attributes[:title] == :manager
						return Manager.new(employee_attributes)
				end
		end
end

Uh oh, the code grew! A lot! Also, we still have a pesky if! We had to put a lot of effort into building the polymorphic employee object.

It looks like we have some design tradeoffs to investigate.

Benefits of this design:

  • Each of our Employee classes is extremely simple.
  • The magic values :manager and :programmer are encapsulated.
  • This code looks easy to extend.

Drawbacks of this design:

  • There are more concepts for the reader grasp.
  • This code is harder to debug. We have to confront the uncertainty of not knowing the type of our employee. Are we dealing with an instance of a Manager when we mean to be dealing with a Programmer?
  • A change to the signature of the hours method will result in one change per type of employee. As the number of Employee classes grows, the signature will become more difficult to change.
  • Unit testing these classes in isolation gives us less confidence that the system works as a whole: we have to worry about their interaction.

The overhead of this design clearly outweighs its benefits.

Asking for Polymorphism

This example is not terribly far from asking for a polymorphic approach. It does not take much to push it over the edge.

Let’s add another method to our Employee class:

class Employee
		attr_accessor :title, :name
		def initialize(employee_attributes)
				@title = employee_attributes[:title]
				@name = employee_attributes[:name]
		end

		def hours
				if title == :programmer
						80
				elsif title == :manager
						40
				end
		end

		def salary
				if title == :programmer
						80000
				elsif title == :manager
						200000
				end
		end
end

Alarm bells should be going off in your head. We have duplication! We also have magic symbols! We all know why this is terrible. A change to one of the duplicated conditionals means changing all of the others. This is likely to become a giant pain. In other words, the code is not “open to extension but closed to modification”.

In this case, refactoring to polymorphism is totally warranted.

Conclusions

As soon as we observed duplication in our solution, it gave us a reason to cope with the indirection introduced by polymorphism. In my opinion, postponing polymorphism is a special case of YAGNI. We decided that the non-polymorphic approach was not "the simplest thing that could possibly work" when we encountered duplication and magic values.

Consider the task of adding a third type of employee, who has both an hours property and a salary property. If we take the non-polymorphic approach, this will be a pain: we will have to undergo the error-prone process of duplicating changes to conditionals. If we take the polymorphic approach, however, we are only required to write a short class. In this respect, the factory justifies its own existence.

It is helpful to think of polymorphism primarily as a tool for reducing duplication; this tool simply happens to occasionally yield elegant abstractions. Instead of creating your classes to model an a priori hierarchy of concepts, use them as a natural way to swiftly clear up duplication.