What Is Replace Conditional with Polymorphism Refactoring? How Is It Implemented in Ruby

What is Replace Conditional with Polymorphism Refactoring? How is it implemented in Ruby?

The Replace Conditional with Polymorphism Refactoring is rather simple and it is pretty much exactly what it sounds like. You have a method with a conditional like this:

def speed
  case @type
  when :european       then base_speed
  when :african        then base_speed - load_factor * @number_of_coconuts
  when :norwegian_blue then if nailed? then 0 else base_speed(@voltage) end
end

and you replace it with polymorphism like this:

class European
  def speed
    base_speed
  end
end

class African
  def speed
    base_speed - load_factor * @number_coconuts
  end
end

class NorwegianBlue
  def speed
    if nailed? then 0 else base_speed(@voltage)
  end
end

You can apply the Refactoring again to NorwegianBlue#speed by creating a subclass of NorwegianBlue:

class NorwegianBlue
  def speed
    base_speed(@voltage)
  end
end

class NailedNorwegianBlue < NorwegianBlue
  def speed
    0
  end
end

Voilà, all your conditionals are gone.

You might ask yourself: does this always work? Can I always replace an if with message dispatch? And the answer is: yes, you can! In fact, if you didn't have if, you can implement it yourself using nothing but message dispatch. (This is, in fact, how Smalltalk does it, there are no conditionals in Smalltalk.)

class TrueClass
  def iff(thn:, els: ->{})
    thn.()
  end

  def &
    yield
  end

  def |
    self
  end

  def !
    false
  end
end

class FalseClass
  def iff(thn:, els: ->{})
    els.()
  end

  def &
    self
  end

  def |
    yield
  end

  def !
    true
  end
end

(3 > 4).iff(thn: ->{ 'three is bigger than four' }, els: ->{ 'four is bigger than three' } )
# => 'four is bigger than three'

true.& { puts 'Hello' }
# Hello

true.| { puts 'Hello' }
# => true

Also relevant: the Anti-IF Campaign

Replace conditional with polymorphism - nice in theory but not practical

You're right - "the conditionals are getting pushed up to the top of the chain" - but there's no "just" about it. It's very powerful. As @thkala says, you just make the choice once; from there on out, the object knows how to go about its business. The approach you describe - BaseAction, ViewAction, and the rest - is a good way to go about it. Try it out and see how much cleaner your code becomes.

When you've got one factory method that takes a string like "View" and returns an Action, and you call that, you have isolated your conditionality. That's great. And you can't properly appreciate the power 'til you've tried it - so give it a shot!

Better way to handle many if conditions?

This code could benefit greatly from the Replace Conditional with Polymorphism Refactoring.

In general, you never need any conditionals at all in an object-oriented language, because object-oriented languages have runtime polymorphic message dispatch, which is more powerful than any conditional. You can always replace conditionals with polymorphism; Smalltalk is the existence proof of that, it doesn't even have conditionals in the language, they are implemented in the library using message dispatch, kind of like this:

class TrueClass
  def if_then_else(then_part, else_part)
    then_part.()
  end
end

class FalseClass
  def if_then_else(then_part, else_part)
    else_part.()
  end
end

(2 < 3).if_then_else(-> { puts 'True' }, -> { puts 'False' })
# True

Ruby does have conditionals, but you don't need them.

So, what does runtime polymorphic message dispatch do, exactly? Well, it basically executes different code based on the type. For example, if you say

foo.bar

different bar method will be ran based on the type of foo.

And in your case, the value you are using to base on your decision which code to execute on, is literally called type, so, you are essentially just re-implementing a basic feature of Ruby: executing different code based on the type is just message dispatch, which Ruby does on its own anyway.

So, in your case, you would have 4 different Feed classes and 2 different Comment classes.

Now, in your case, it is a little bit more complicated because the outcome does not depend solely on the feed type, but on the comment type as well. Ruby doesn't have multiple dispatch, so, we will either need to introduce new classes for every combination of feed type and comment type, or live with some conditionals in the final code.

So, let's start slowly improving your code. First off, I believe you mean elsif instead of else if in your code:

if feed.type == 1
  if nested_comment
    str = "test"
  elsif comment
    str = "test1"
  else
    str = "test2"
  end 
elsif feed.type == 2
  if nested_comment
    str = "test3"
  elsif comment
    str = "test4"
  else
    str = "test5"
  end 
elsif feed.type == 3
  if nested_comment
    str = "test6"
  elsif comment
    str = "test7"
  else
    str = "test8"
  end 
elsif feed.type == 4
  if nested_comment
    str = "test9"
  elsif comment
    str = "test10"
  else
    str = "test11"
  end 
end

Secondly, we can make use of the fact that conditionals in Ruby are expressions, not statements (in fact, everything in Ruby is an expression, there are no statements), and thus evaluate to a value:

str = if feed.type == 1
  if nested_comment
    "test"
  elsif comment
    "test1"
  else
    "test2"
  end 
elsif feed.type == 2
  if nested_comment
    "test3"
  elsif comment
    "test4"
  else
    "test5"
  end 
elsif feed.type == 3
  if nested_comment
    "test6"
  elsif comment
    "test7"
  else
    "test8"
  end 
elsif feed.type == 4
  if nested_comment
    "test9"
  elsif comment
    "test10"
  else
    "test11"
  end 
end

Now, we replace those ifs with case expressions:

str = case feed.type
when 1
  case
  when nested_comment
    "test"
  when comment
    "test1"
  else
    "test2"
  end 
when 2
  case
  when nested_comment
    "test3"
  when comment
    "test4"
  else
    "test5"
  end 
when 3
  case
  when nested_comment
    "test6"
  when comment
    "test7"
  else
    "test8"
  end 
when 4
  case
  when nested_comment
    "test9"
  when comment
    "test10"
  else
    "test11"
  end 
end

Now, let's reformat a little, to easier see what's going on:

str = case feed.type
when 1
  case
  when nested_comment then "test"
  when comment        then "test1"
  else                     "test2"
  end 
when 2
  case
  when nested_comment then "test3"
  when comment        then "test4"
  else                     "test5"
  end 
when 3
  case
  when nested_comment then "test6"
  when comment        then "test7"
  else                     "test8"
  end 
when 4
  case
  when nested_comment then "test9"
  when comment        then "test10"
  else                     "test11"
  end 
end

It's time for our refactoring:

class Feed1
  def magic_string
    case
    when nested_comment then "test"
    when comment        then "test1"
    else                     "test2"
    end
  end
end

class Feed2
  def magic_string
    case
    when nested_comment then "test3"
    when comment        then "test4"
    else                     "test5"
    end
  end
end

class Feed3
  def magic_string
    case
    when nested_comment then "test6"
    when comment        then "test7"
    else                     "test8"
    end
  end
end

class Feed4
  def magic_string
    case
    when nested_comment then "test9"
    when comment        then "test10"
    else                     "test11"
    end
  end 
end

str = feed.magic_string

We can further reduce some duplication by introducing a method which encapsulates the comment checking (or, like I said, we can introduce comment classes).

class Feed
  def comment_string(nested_comment_string, comment_string, other_string)
    case
    when nested_comment then nested_comment_string
    when comment        then comment_string
    else                     other_string
    end
  end
end

class Feed1 < Feed
  def magic_string
    comment_string("test", "test1", "test2")
  end
end

class Feed2 < Feed
  def magic_string
    comment_string("test3", "test4", "test5")
  end
end

class Feed3 < Feed
  def magic_string
    comment_string("test6", "test7", "test8")
  end
end

class Feed4 < Feed
  def magic_string
    comment_string("test9", "test10", "test11")
  end 
end

str = feed.magic_string

Correct way to TDD methods that calls other methods

I'm presuming since you mention TDD the code in question does not actually exist. If it does then you aren't doing true TDD but TAD (Test-After Development), which naturally leads to questions such as this. In TDD we start with the test. It appears that you are building some type of menu or command system, so I'll use that as an example.

describe GameMenu do
  it "Allows you to navigate to character creation" do
    # Assuming character creation would require capturing additional
    # information it violates SRP (Single Responsibility Principle)
    # and belongs in a separate class so we'll mock it out.
    character_creation = mock("character creation")
    character_creation.should_receive(:execute)

    # Using constructor injection to tell the code about the mock
    menu = GameMenu.new(character_creation)
    menu.execute("c")
  end
end

This test would lead to some code similar to the following (remember, just enough code to make the test pass, no more)

class GameMenu
  def initialize(character_creation_command)
    @character_creation_command = character_creation_command
  end

  def execute(command)
    @character_creation_command.execute
  end
end

Now we'll add the next test.

it "Allows you to display character inventory" do
  inventory_command = mock("inventory")
  inventory_command.should_receive(:execute)
  menu = GameMenu.new(nil, inventory_command)
  menu.execute("i")
end

Running this test will lead us to an implementation such as:

class GameMenu
  def initialize(character_creation_command, inventory_command)
    @inventory_command = inventory_command
  end

  def execute(command)
    if command == "i"
      @inventory_command.execute
    else
      @character_creation_command.execute
    end
  end
end

This implementation leads us to a question about our code. What should our code do when an invalid command is entered? Once we decide the answer to that question we could implement another test.

it "Raises an error when an invalid command is entered" do
  menu = GameMenu.new(nil, nil)
  lambda { menu.execute("invalid command") }.should raise_error(ArgumentError)
end

That drives out a quick change to the execute method

  def execute(command)
    unless ["c", "i"].include? command
      raise ArgumentError("Invalid command '#{command}'")
    end

    if command == "i"
      @inventory_command.execute
    else
      @character_creation_command.execute
    end
  end

Now that we have passing tests we can use the Extract Method refactoring to extract the validation of the command into an Intent Revealing Method.

  def execute(command)
    raise ArgumentError("Invalid command '#{command}'") if invalid? command

    if command == "i"
      @inventory_command.execute
    else
      @character_creation_command.execute
    end
  end

  def invalid?(command)
    !["c", "i"].include? command
  end

Now we finally got to the point we can address your question. Since the invalid? method was driven out by refactoring existing code under test then there is no need to write a unit test for it, it's already covered and does not stand on it's own. Since the inventory and character commands are not tested by our existing test, they will need to be test driven independently.

Note that our code could be better still so, while the tests are passing, lets clean it up a bit more. The conditional statements are an indicator that we are violating the OCP (Open-Closed Principle) we can use the Replace Conditional With Polymorphism refactoring to remove the conditional logic.

# Refactored to comply to the OCP.
class GameMenu
  def initialize(character_creation_command, inventory_command)
    @commands = {
      "c" => character_creation_command,
      "i" => inventory_command
    }
  end

  def execute(command)
    raise ArgumentError("Invalid command '#{command}'") if invalid? command
    @commands[command].execute
  end

  def invalid?(command)
    !@commands.has_key? command
  end
end

Now we've refactored the class such that an additional command simply requires us to add an additional entry to the commands hash rather than changing our conditional logic as well as the invalid? method.

All the tests should still pass and we have almost completed our work. Once we test drive the individual commands you can go back to the initialize method and add some defaults for the commands like so:

  def initialize(character_creation_command = CharacterCreation.new,
                 inventory_command = Inventory.new)
    @commands = {
      "c" => character_creation_command,
      "i" => inventory_command
    }
  end

The final test is:

describe GameMenu do
  it "Allows you to navigate to character creation" do
    character_creation = mock("character creation")
    character_creation.should_receive(:execute)
    menu = GameMenu.new(character_creation)
    menu.execute("c")
  end

  it "Allows you to display character inventory" do
    inventory_command = mock("inventory")
    inventory_command.should_receive(:execute)
    menu = GameMenu.new(nil, inventory_command)
    menu.execute("i")
  end

  it "Raises an error when an invalid command is entered" do
    menu = GameMenu.new(nil, nil)
    lambda { menu.execute("invalid command") }.should raise_error(ArgumentError)
  end
end

And the final GameMenu looks like:

class GameMenu
  def initialize(character_creation_command = CharacterCreation.new,
                 inventory_command = Inventory.new)
    @commands = {
      "c" => character_creation_command,
      "i" => inventory_command
    }
  end

  def execute(command)
    raise ArgumentError("Invalid command '#{command}'") if invalid? command
    @commands[command].execute
  end

  def invalid?(command)
    !@commands.has_key? command
  end
end

Hope that helps!

Brandon

Short way to write a long case statement

I don't understand some of the details of the question, but here's a general approach you might take. I've assumed that :info and the values of :email in the hash below are names of methods. (I understand that assumption is incorrect.) The following may have errors, considering that I have no means of testing it.

DATA = [[/osha/,    'Creating OSHA Regional email..',                :osha_reg],
        [/pend/,    'Creating 6 day hold pending email..',           :pend],
        [/60/,      'Creating 60 day hold account deletion email..', :sixty_day],
        [/generic/, 'Creating generic email..',                      :generic],
        [/resolve/, 'Creating resolution ticket..',                  :resolve],
        [/esc/,     'Creating escalation ticket..',                  :assign],
        [/pii/,     'Creating request to remove personal info..',    :remove_pii],
        [/vip/,     'Creating VIP user email..',                     :vip_user],
        [/inop/,    'Creating INOP user email..',                    :in_op_user]]

def gather_intel
  type = OPTIONS[:type]
  regex, msg, email = DATA.find { |r,*_| type =~ r }
  if regex
    FORMAT.send :info, msg
    EMAILS.send email
  elsif type =~ /dev/ 
    message = type.to_s.include?('dev=unlock') ? 'unlock' : 'password reset'
    FORMAT.info("Creating dev account #{message} email")
    EMAILS.dev_account(type)
  else
    raise ERROR
  end
end

If-less programming (basically without conditionals)

There are some resources on the Anti-IF Campaign site, such as this article.

I believe it's a matter of degree. Conditionals aren't always bad, but they can be (and frequently are) abused.

Additional thoughts (one day later)

Refactoring: Improving the Design of Existing Code is a good reference on this subject (and many others). It covers Replace Conditional with Polymorphism. There's also a new one, Replace Conditional with Visitor, on the web site.

I value simplicity and single responsibility over removing all if statements. These three goals often coincide. Static analysis tools that support the cyclomatic complexity metric can quickly point out code with nested or serial conditionals. The if statements may remain post-refactoring, but could be broken out into smaller methods and/or multiple classes.

Update: Michael Feathers wrote an article on Unconditional Programming.

This is a popular topic: Phil Haack on Death to the IF statement!

Replacing if else statement with pattern

This is a classic Replace Condition dispatcher with Command in the Refactoring to Patterns book.

Basically you make a Command object for each of the blocks of code in your old if/else group and then make a Map of those commands where the keys are your condition Strings

interface Handler{
    void handle( myObject o);
}

 Map<String, Handler> commandMap = new HashMap<>();
 //feel free to factor these out to their own class or
 //if using Java 8 use the new Lambda syntax
 commandMap.put("conditionOne", new Handler(){
         void handle(MyObject o){
                //get desired parameters from MyObject and do stuff
          }
 });
 ...

Then instead of your if/else code it is instead:

 commandMap.get(someCondition).handle(this);

Now if you need to later add new commands, you just add to the hash.

If you want to handle a default case, you can use the Null Object pattern to handle the case where a condition isn't in the Map.

 Handler defaultHandler = ...

if(commandMap.containsKey(someCondition)){
    commandMap.get(someCondition).handle(this);
}else{
    defaultHandler.handle(this);
}

Why is the 'if' statement considered evil?

The if statement is rarely considered as "evil" as goto or mutable global variables -- and even the latter are actually not universally and absolutely evil. I would suggest taking the claim as a bit hyperbolic.

It also largely depends on your programming language and environment. In languages which support pattern matching, you will have great tools for replacing if at your disposal. But if you're programming a low-level microcontroller in C, replacing ifs with function pointers will be a step in the wrong direction. So, I will mostly consider replacing ifs in OOP programming, because in functional languages, if is not idiomatic anyway, while in purely procedural languages you don't have many other options to begin with.

Nevertheless, conditional clauses sometimes result in code which is harder to manage. This does not only include the if statement, but even more commonly the switch statement, which usually includes more branches than a corresponding if would.

There are cases where it's perfectly reasonable to use an if

When you are writing utility methods, extensions or specific library functions, it's likely that you won't be able to avoid ifs (and you shouldn't). There isn't a better way to code this little function, nor make it more self-documented than it is:

// this is a good "if" use-case
int Min(int a, int b)
{
    if (a < b) 
       return a;
    else
       return b;
}

// or, if you prefer the ternary operator
int Min(int a, int b)
{
    return (a < b) ? a : b;
}

Branching over a "type code" is a code smell

On the other hand, if you encounter code which tests for some sort of a type code, or tests if a variable is of a certain type, then this is most likely a good candidate for refactoring, namely replacing the conditional with polymorphism.

The reason for this is that by allowing your callers to branch on a certain type code, you are creating a possibility to end up with numerous checks scattered all over your code, making extensions and maintenance much more complex. Polymorphism on the other hand allows you to bring this branching decision as closer to the root of your program as possible.

Consider:

// this is called branching on a "type code",
// and screams for refactoring
void RunVehicle(Vehicle vehicle)
{
    // how the hell do I even test this?
    if (vehicle.Type == CAR)
        Drive(vehicle);
    else if (vehicle.Type == PLANE)
        Fly(vehicle);
    else
        Sail(vehicle);
}

By placing common but type-specific (i.e. class-specific) functionality into separate classes and exposing it through a virtual method (or an interface), you allow the internal parts of your program to delegate this decision to someone higher in the call hierarchy (potentially at a single place in code), allowing much easier testing (mocking), extensibility and maintenance:

// adding a new vehicle is gonna be a piece of cake
interface IVehicle
{
    void Run();
}

// your method now doesn't care about which vehicle 
// it got as a parameter
void RunVehicle(IVehicle vehicle)
{
    vehicle.Run();
}

And you can now easily test if your RunVehicle method works as it should:

// you can now create test (mock) implementations
// since you're passing it as an interface
var mock = new Mock<IVehicle>();

// run the client method
something.RunVehicle(mock.Object);

// check if Run() was invoked
mock.Verify(m => m.Run(), Times.Once());

Patterns which only differ in their if conditions can be reused

Regarding the argument about replacing if with a "predicate" in your question, Haines probably wanted to mention that sometimes similar patterns exist over your code, which differ only in their conditional expressions. Conditional expressions do emerge in conjunction with ifs, but the whole idea is to extract a repeating pattern into a separate method, leaving the expression as a parameter. This is what LINQ already does, usually resulting in cleaner code compared to an alternative foreach:

Consider these two very similar methods:

// average male age
public double AverageMaleAge(List<Person> people)
{
    double sum = 0.0;
    int count = 0;
    foreach (var person in people)
    {
       if (person.Gender == Gender.Male)
       {
           sum += person.Age;
           count++;
       }
    }
    return sum / count; // not checking for zero div. for simplicity
}

// average female age
public double AverageFemaleAge(List<Person> people)
{
    double sum = 0.0;
    int count = 0;
    foreach (var person in people)
    {
       if (person.Gender == Gender.Female) // <-- only the expression
       {                                   //     is different
           sum += person.Age;
           count++;
       }
    }
    return sum / count;
}

This indicates that you can extract the condition into a predicate, leaving you with a single method for these two cases (and many other future cases):

// average age for all people matched by the predicate
public double AverageAge(List<Person> people, Predicate<Person> match)
{
    double sum = 0.0;
    int count = 0;
    foreach (var person in people)
    {
       if (match(person))       // <-- the decision to match
       {                        //     is now delegated to callers
           sum += person.Age;
           count++;
       }
    }
    return sum / count;
}

var males = AverageAge(people, p => p.Gender == Gender.Male);
var females = AverageAge(people, p => p.Gender == Gender.Female);

And since LINQ already has a bunch of handy extension methods like this, you actually don't even need to write your own methods:

// replace everything we've written above with these two lines
var males = list.Where(p => p.Gender == Gender.Male).Average(p => p.Age);
var females = list.Where(p => p.Gender == Gender.Female).Average(p => p.Age);

In this last LINQ version the if statement has "disappeared" completely, although:

1. to be honest the problem wasn't in the if by itself, but in the entire code pattern (simply because it was duplicated), and
2. the if still actually exists, but it's written inside the LINQ Where extension method, which has been tested and closed for modification. Having less of your own code is always a good thing: less things to test, less things to go wrong, and the code is simpler to follow, analyze and maintain.

Huge runs of nested if/else statements

When you see a function spanning 1000 lines and having dozens of nested if blocks, there is an enormous chance it can be rewritten to

1. use a better data structure and organize the input data in a more appropriate manner (e.g. a hashtable, which will map one input value to another in a single call),
2. use a formula, a loop, or sometimes just an existing function which performs the same logic in 10 lines or less (e.g. this notorious example comes to my mind, but the general idea applies to other cases),
3. use guard clauses to prevent nesting (guard clauses give more confidence into the state of variables throughout the function, because they get rid of exceptional cases as soon as possible),
4. at least replace with a switch statement where appropriate.

Refactor when you feel it's a code smell, but don't over-engineer

Having said all this, you should not spend sleepless nights over having a couple of conditionals now and there. While these answers can provide some general rules of thumb, the best way to be able to detect constructs which need refactoring is through experience. Over time, some patterns emerge that result in modifying the same clauses over and over again.

---

Related Topics