The Syntax of Taste: Refactoring Conditionals

Conditionals aren’t inherently bad, but they sure do have a vexing tendency to proliferate. Much like fungus, they thrive and multiply in dark and dank places like, say, codebases.

Here we’ll look at a typical, home-grown conditional and look at how to gain a small measure of control.

Catering to Conflicting Preferences

I have an imaginary app. It runs on the command-line and streams music, and it’s so simple to use that even my non-programmer friends use it:

$ fm

That’s it.

By default, it plays a curated list of music, but that’s not always appropriate, or even desirable. In order to allow for a variety of different social situations and the odd bout of nostalgia, fm supports an additional syntax:

$ fm <GENRE> # e.g. acid-jazz

Finally, there’s the “Oh, I don’t know, surprise me” mode:

$ fm mixtape

mixtape is a mostly delightful blend of whatever it can source from any of a number of locations, local and remote.

The biggest complaint about fm is that the mixtape command includes genres that people don’t like. As you might expect, no two people complain about the same genres.

Complaints come in two general flavors. Some people are perfectly happy with extreme variety, they simply want to filter out some very specific genres that rub them the wrong way. Others are more particular about their music and want to be surprised only within a very narrow set of comfortable and familiar genres.

Oh, we got both kinds [of music here], we got country and western. –The Blues Brothers

Expanding the API

In order to allow for different preferences, the Stream object, which manages the play list, needs to know whether or not a genre is enabled:

class Stream
  def genre_enabled?
    # magic happens here
  end
end

The fm codebase isolates configuration into a single Config object, which the stream has access to:

Stream.new(Config.new)

Here’s the boilerplate that you’ll need in order to follow along at home.

# lib/fm/config.rb
module FM
  class Config
    # ...
  end
end

# lib/fm/stream.rb
module FM
  class Stream
    attr_reader :config
    def initialize(config)
      @config = config
    end

    # ...
  end
end

# test/fm/stream_test.rb
gem 'minitest', '~> 5.4'
require 'minitest/autorun'
require './lib/fm/stream'
require './lib/fm/config'

class StreamTest < Minitest::Test
  # ...
end

The tests can be run using the following command:

ruby test/fm/stream_test.rb

Ensuring Backwards Compatibility

The app still has to work for those who don’t bother configuring the mixtape feature at all. In this case the mixtape command should play anything and everything. No matter what genre comes up in the stream, it should be enabled:

def test_all_genres_enabled_by_default
  stream = FM::Stream.new(FM::Config.new)

  assert stream.genre_enabled?(:abc), "expected abc to be enabled"
  assert stream.genre_enabled?(:xyz), "expected xyz to be enabled"
end

A trivial amount of code gets the test passing:

# lib/fm/stream.rb
def genre_enabled?(genre)
  true
end

At this point, there’s no way to move forward without thinking about how people will restrict the genres.

Choosing a UI

A fairly idiomatic UI would look something like this for whitelisting genres:

$ fm configure --only jazz,blues,power-metal,trance

For the less picky listeners, the blacklisting feature would look like this:

$ fm configure --except house,opera

Unfortunately, this type of UI is going to fail in spectacular ways. Forcing non-programmers to deal with command-line options is just mean.

A better UI would be no UI at all.

Since this is not an app that is going to be distributed widely, I can simply spend a few minutes setting everything up for the handful of people in question.

The amount of programming necessary to get no UI working is negligible. There’s no need to muck about with taking arguments, writing to files, loading JSON or YAML, or interactive prompts.

Environment variables are simple enough to deal with, and non-programmers have no problem editing text files:

ENV['FM_GENRE_WHITELIST'] = 'abc,def,ghi,jkl'

Enabling and Disabling Genres

Environment variables are global, which means that the tests have the potential to interfere with each other. They need to meticulously clean up after themselves:

def test_enabled_via_whitelist
  ENV['FM_GENRE_WHITELIST'] = 'abc, def,ghi , jkl'
  # ...
ensure
  ENV.delete('FM_GENRE_WHITELIST')
end

The inconsistent whitespace in the list of genres is to take into account that few non-programmers notice the difference between “apples,bananas,cherries” and “apples, bananas, cherries”.

We’ll need a Stream object to do all the work:

stream = FM::Stream.new(FM::Config.new)

Finally, we’ll need two types of assertions:

1. Everything that is defined in the list is turned on.

2. Something not on the list is turned off.

   %i(abc def ghi jkl).each do |genre|
     assert stream.genre_enabled?(genre), "expected '#{genre}' to be enabled"
   end
   
   refute stream.genre_enabled?(:xyz), "expected xyz to be disabled"

It doesn’t take much tinkering to get the test passing:

# lib/fm/config.rb
def included_genres
  @included_genres ||= ENV['FM_GENRE_WHITELIST'].to_s.split(/\s*,\s*/).map { |s| s.strip.to_sym }
end

# lib/fm/stream.rb
def genre_enabled?(genre)
  return true if config.included_genres.empty?

  config.included_genres.include?(genre)
end

The blacklist is pretty much the same thing, with the assertions reversed:

def test_disabled_via_blacklist
  ENV['FM_GENRE_BLACKLIST'] = 'abc, def,ghi , jkl'
  stream = FM::Stream.new(FM::Config.new)

  %i(abc def ghi jkl).each do |genre|
    refute stream.genre_enabled?(genre), "expected '#{genre}' to be disabled"
  end

  assert stream.genre_enabled?(:xyz), "expected xyz to be enabled"
ensure
  ENV.delete('FM_GENRE_BLACKLIST')
end

The configuration is straight forward:

# lib/fm/config.rb
def included_genres
  @included_genres ||= tokenize(ENV['FM_GENRE_WHITELIST'])
end

def excluded_genres
  @excluded_genres ||= tokenize(ENV['FM_GENRE_BLACKLIST'])
end

private

def tokenize(s)
  s.to_s.split(/\s*,\s*/).map { |s| s.strip.to_sym }
end

The Stream object, though, is suddenly quite a bit more complex:

# lib/fm/stream.rb
def genre_enabled?(genre)
  if config.excluded_genres.empty? && config.included_genres.empty?
    return true
  end
  if !config.included_genres.empty?
    return config.included_genres.include?(genre)
  end
  !config.excluded_genres.include?(genre)
end

If you’re even slightly boolean impaired, this code is not immediately obvious.

Removing Unnecessary Complexity

Right now we are handling three cases, but the not configured case can be seen as a special case of blacklisting where nothing is blacklisted.

In other words, if the whitelist has something in it, then we’re in whitelist mode, otherwise we can safely assume that we’re dealing with a blacklist, which may or may not be empty.

That simplifies things considerably:

def genre_enabled?(genre)
  if !config.included_genres.empty?
    return config.included_genres.include?(genre)
  end
  !config.excluded_genres.include?(genre)
end

This isn’t awful.

OK, so it’s totally awful, but at least it’s not very long. I don’t think anyone would complain about finding code like this in their production codebase.

Notice the repeating suffix in the method names on config: included_genres and excluded_genres. It seems like perhaps we’re dealing with one type of thing, genres, and that it comes in two flavours: included and excluded.

Or, if you will, whitelists and blacklists.

What if you always knew that you were talking to genres, but didn’t know or care how they were implemented?

genres.enabled?(:opera)

This could be a single object that wraps the conditional:

class Genres
  attr_reader :values
  def initialize(list)
    @values = list.to_s.split(/\s*,\s*/).map { |s| s.strip.to_sym }
  end

  def enabled?(genre)
    if !config.included_genres.empty?
      return config.included_genres.include?(genre)
    end
    !config.excluded_genres.include?(genre)
  end
end

Or you might have two objects, one for whitelists, and one for blacklists:

class BlacklistedGenres
  # ...
  def enabled?(genre)
    !values.include?(genre.to_sym)
  end
end

class WhitelistedGenres
  # ...
  def enabled?(genre)
    values.include?(genre.to_sym)
  end
end

Suddenly there’s no need to know the flavor of the genre. This might be my grandmother’s configuration, full of funk and soul and rock:

genres = WhitelistedGenres.new("funk,soul,rock")

Or it could be that of the 12-year old drummer who lives next door and will listen to anything, provided that it contains some sort of beat:

genres = BlacklistedGenres.new("gregorian-chants")

Even with two itty-bitty objects, the conditional hasn’t disappeared entirely. It’s still there, in the Config, but now the question isn’t how is this configured and given this particular state, how do I query for the right value?, the question is what object knows how to solve this problem?.

All of the complexity of catering to people’s taste in music now boils down to creating the right object:

# lib/fm/config.rb
def genres
  @genres ||= if !!ENV['FM_GENRE_WHITELIST']
    WhitelistedGenres.new(ENV['FM_GENRE_WHITELIST'])
  else
    BlacklistedGenres.new(ENV['FM_GENRE_BLACKLIST'])
  end
end

Once you’re holding on to a genres instance, it really doesn’t matter what it does or how it does it. All it needs to do is respond to one simple question:

genres.enabled?(:jazz)

Recognizing a Potential Refactoring

In Martin Fowler’s book, Refactoring there are a number of refactorings that are about turning conditionals into objects. They have fancy names, and it can seem like it’s all very advanced refactor-fu.

It’s not.

Rearranging code is generally pretty trivial. The tricky part is knowing where to begin, and recognizing that you can do it.

Part of getting over that barrier is simply doing it a few times. Find a conditional, and for each of its blocks, create an object whose sole responsibility is to do that thing.

Will this always be the right choice? Absolutely not. But give it a whirl. Try it in big apps and small apps, with big conditionals and small conditionals. Get a feel for what it can do for you, and where the gotchas are.

If it makes you feel better, throw the code away when you’re done. The point isn’t to turn this particular conditional into objects, but to know how to turn some conditionals into objects.

Knowing if you should refactor your if comes from practice.