coding without ifs

Posted by jcarroll

May 01

The thing that complicates code more than anything else is conditional logic; ‘ifs’, ‘elses’, ‘unlesses’, nested and un-nested. I always try to eliminate them anyway I can because without them, the code is easier to understand and has a better rhythm. For instance:

1
2
3
4
5
6
7
8
9

  def update_subscriptions
    Subscription.find(:all).each do |each|
      if each.expired?
        each.renew!
      else
        each.update!
      end
    end
  end

vs.

1
2
3
4
5
6
7

  def update_subscriptions
    subscriptions = Subscription.find :all
    expired = subscriptions.select {|each| each.expired?}
    expired.each {|each| each.renew!}
    active = subscriptions.reject {|each| each.expired?}
    active.each {|each| each.update!}
  end

In the above code, we let Enumerable’s #select and #reject methods take care of the conditional logic, i.e. we moved the conditional logic into Ruby and out of our code. Look at how in the second example the code just flows up and down, no indenting and very easy to read. Even non-programmers would choose the second example as being more straightforward and elegant.

Another example.

Say controller filters in Rails worked like this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

  class EventsController < ApplicationController

    def new
    end

    def create
    end

    def index
    end

    def show
    end

   private

    def filter
      if action_name == 'new' ||
           action_name == 'create'
         authenticate
      end
    end

    # or a la Searching Literal
    def filter
      if %w(new create).include?(action_name)
        authenticate
      end
    end

    def authenticate
      unless session[:user_id]
        redirect_to login_url
      end
    end

  end

In our #filter method we need conditional logic because we only want to require the user to be logged in for the #new and #create actions. However, Rails’ filter class methods support an ‘only’ keyword parameter. So we can rewrite it using the #before_filter class method instead:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

  class EventsController < ApplicationController

    before_filter :authenticate, :only => [:new, create]

    def new
    end

    def create
    end

    def index
    end

    def show
    end

   private

    def authenticate
      unless session[:user_id]
        redirect_to login_url
      end
    end

  end

Very nice. This time we moved the conditional logic into the framework. Now somewhere in Rails’ #before_filter there’s an ‘if’ statement and 1 less ‘if’ in our code.

ActiveRecord::Base validations also support this. Say that when registering for our site, age is optional, but if you do provide an age, it has to be between 18 and 30.

1
2
3
4
5
6
7
8
9
10
11

  class User < ActiveRecord::Base

    def validate
      if ! age.blank?
         if ! (18..30).include?(age)
           errors.add :age, 'must be between 18 and 30'
         end
      end
    end

  end

I don’t like it, look at all the conditional logic. Fortunately ActiveRecord::Base’s validation class methods can help us out:

1
2
3
4
5
6
7
8

  class User < ActiveRecord::Base

    validates_inclusion_of :age,
      :allow_nil => true,
      :in => 18..30,
      :message => 'must be between 18 and 30'

  end

Much better. Now Rails’ #validates_inclusion_of has the 2 ‘if’ statements and our code has 0.

Interestingly, Rails’ ActiveRecord::Base callbacks do not support an ‘if’ parameter.

For example, say we have publications in our application. And we support 2 types of publications call them ‘ABC’ and ‘XYZ’. Now the information for ‘ABC’ and ‘XYZ’ publications are provided by their respective sites, so we have to request the publication information from them via HTTP. We store it locally to avoid looking it up remotely every time we display a publication on our site. And that we don’t have to worry about the data getting out of sync because the remote data will never change.

1
2
3
4
5
6
7
8
9
10
11
12

  class Publication < ActiveRecord::Base

    def before_create
      if type == 'ABC'
        self.attributes = AbcGateway.find name # the publication name is enough to uniquely identify itself
      end
      if type == 'XYZ'
        self.attributes = XyzGateway.find name # the publication name is enough to uniquely identify itself
      end
    end

  end
More ugly conditionals. I wish Rails would support the following (using the #before_create class method instead of overriding the instance method): 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

  class Publication < ActiveRecord::Base

    before_create :abc, :if => Proc.new {|publication| publication.type == 'ABC'}

    before_create :xyz, :if => Proc.new {|publication| publication.type == 'XYZ'}

    def abc
      self.attributes = AbcGateway.find name
    end

    def xyz
      self.attributes = XyzGateway.find name
    end

  end

That’s much tighter without the conditionals.

One piece of Ruby syntactic sugar that I don’t care for are ‘if’ and ‘unless’ modifiers. My first example could be re-written more compactly using them like so:

1
2
3
4
5
6

  def update_subscriptions
    Subscription.find(:all).each do |each|
      each.renew! if each.expired?
      each.update! unless each.expired?
    end
  end

I don’t like this style because the code now flows as if there’s no conditional logic. The ‘if’ and ‘unless’ modifiers have hidden the obviousness of conditional logic, i.e. the indenting. I like to see the conditional logic because it makes the code look ugly and its always there reminding me to think about how to refactor its complicatedness and ugliness out of there.

The same goes for the old ternary operator:

1
2
3
4
5

  def update_subscriptions
    Subscription.find(:all).each do |each|
      each.expired? ? each.renew! : each.update!
    end
  end

Even shorter. But once again the conditional logic is not obvious. Also the double ’?’ is ugly since Ruby allows ’?’ in method names; this makes using the ternary operator not as elegant as in other languages.

Comments on this post

Eric Mill

May 01

Eric Mill said,

“Look at how in the second example the code just flows up and down, no indenting and very easy to read. Even non-programmers would choose the second example as being more straightforward and elegant.”

I disagree. The first block of code has only 3 pieces of logic in it after the initial Subscription.find. The second block of code is dense, with four lines, where each line calls a function and passes a block with another function call within. That’s 8 function calls, to 3.

This second block is also much more expensive. The first block creates the array and traverses it once. The second block creates the array and traverses it three times.

The second block is much less readable, and as far as non-programmers go, would be significantly more intimidating. Do you think humans don’t understand conditionals naturally? It’s how we speak. The first block is read as “For each subscription, if it’s expired, renew it, otherwise update it.”. The second block reads “Find all subscriptions, then pick out the expired ones. Renew the expired ones. Then pick out the non-expired ones, and update the non-expired ones.”

Your method requires no mental “stack”, but requires more time, code, cycles, and distances Ruby code from its natural (and amazing) concordance with human thought.

Jon Yurek

May 01

Jon Yurek said,

I can get behind the idea of using the select/reject (or, as would be far superior in this case, partition) for certain problems, but not for the purpose of getting rid of conditionals, mostly because you still have a conditional statement, you’re just stating it differently. The first block of code is the right way to do it. It’s simple, straightforward, and readable. The second block of code is, honestly, just a lump of code. It’s denser, wordier, less readable, less performant, it does the same thing with more complex logic, and requires more changes in more places if refactoring is needed.

I do agree with you about the conditionals in the validations and filters, but, again, you haven’t actually gotten rid of the conditionals, you’ve just relocated them to someone else’s code. The complexity is still there, you’ve just moved it.

Frankly, I think that if statements are such a fundamental part of programming that it’s a bit shortsighted to try to get rid of them, especially considering that what you’re trying to replace them with requires them.

Floyd

May 01

Floyd said,

One could easily say about blocks what you say about conditionals. “Never use blocks because they’re too confusing, use conditionals instead” and give your example in reverse. You are simply stating a completely subjective preference, a mere opinion, as if it had the weight of objective fact. And it happens to be one that I strongly disagree with. You can’t say it’s less code; it’s not. You can’t even say it’s more familiar to the majority if programmers; blocks are by far the more esoteric language construct. In sum, I find your argument completely untenable. It’s just another misguided pattern the value of which is completely dependent on the skill and artistry of the programmer making use of it.

But keep tossing them out here on the bonfire, I like watching ‘em burn.

jare care

May 01

jare care said,

This underlying concept of this article is about putting responsibilities where they belong and letting objects do what they’re responsible for.

Array’s support conditional iteration with #select and #reject, by putting ‘if’ and ‘else’ in an #each block you’re doing work that Array already does and does correctly, so you can avoid your own bugs. Tell the object to do the work for you, don’t ask with your own conditional logic.

Validation typically involves sentences such as ‘if they provide an age then make sure its between 18-30’. Therefore the object in charge of validation should take care of the conditional logic for you. And that’s what methods like #validates_inclusion_of do and do correctly, so you can avoid your own validation’s bugs. Once again tell the object to do the work for you and avoid asking questions i.e. ‘if’ statements.

“Tell don’t ask”

Floyd

May 01

Floyd said,

Your twisting the current example (and any hypothetical which employs a conditional statement) to appear invalid in light of the tell don’t ask pattern is a perfect example of how patterns corrupt programming.

jare care

May 01

jare care said,

“Tell don’t ask” is a fundamental principle of good design, less conditional logic is a result of applying it; its not a pattern.

“DRY” is another principle; there’s no “DRY pattern”.

Avoiding conditional logic will help you achieve “tell don’t ask” and result in code that asks (‘if’s) less questions; which is much easier to maintain.

It also demonstrates that a developer understands objects. Telling and not asking is about moving from procedural (non-OO) to object-oriented (OO) development. The examples demonstrate how you can write C in Ruby and then how to write Ruby in Ruby. No more ‘ifs’, its time to move on and lets objects do their thing.

Floyd

May 01

Floyd said,

Alright, I’ll let you have the last word, as long as you define a programming “principle” as distinct from a programming “pattern”.

labrat

May 01

labrat said,

I’ve got to agree with others and say that the “refactored” example looks like a mess. Naming the instances “each” also adds to confusion.

This might not be much for a contrived blog example but if this was the style in a substantial code base that I had to get used to my brain would explode a couple times.

Conditionals are built into the very DNA of programming starting with whether it’s a 1 or 0 at the most fundamental level. Although I would agree that if you have a massive method that looks like a complicated miniature flow chart, you should re-examine your design.

jare care

May 01

jare care said,

A principle is very generic e.g. “tell don’t ask”, “dry”, “open-closed principle”, “single responsibility principle”. These can apply to any part of your application e.g. ‘presentation (i.e. view logic)’, ‘persistence’, ‘domain logic’, etc. And there are many different ways to achieve those principles.

A pattern is a problem and a solution in a context e.g ‘strategy’, ‘state’, ‘single table inheritance’, ‘active record’. A pattern is less generic because it deals with a more specific problem in your application e.g. ‘pluggable algorithms with strategy’, ‘pluggable states with state’, ‘mapping object inheritance to database tables with single table inheritance’, ‘mapping objects to database tables with active record’. However it, can still be implemented in a number of different ways.

Piers Cawley

May 01

Piers Cawley said,

That first example is really rather horrible you know. Replace your expired attribute with a state object and let polymorphism be your friend.

Rails makes things a little harder than it could be in the way that composed_of works, but with a little bit of fancy footwork you can do:

class Subscription < ActiveRecord::Base
  composed_of :state, :mapping => %(state_memento), :class_name => 'Subscription::StateBuilder'

  class StateBuilder
    def new(memento)
      memento.constantize.new
    end
  end

  class State
    def memento
      self.class.to_s
    end
  end

  class Expired < State
    def update(subscription)
      subscription.renew!
    end

    def expired?
      true
    end
  end

  class Active < State
    def update(subscription)
      subscription.update!
    end

    def expired?
      false
    end
  end

  def expired?
    state.expired?
  end

  # this has to be the wrong method name. #refresh perhaps?
  def refresh
    state.update(self)
  end

  def self.update_all
    self.find(:all).each &:update
  end
end

Then your update_subscriptions method becomes a simple call to a class method on Subscription.

And yes, I know there’s a good deal of complexity in the code here, but quite a bit of it is boilerplate and can be cleared up behind a handful of friendly class methods. A few helpers along the lines of

class Subscription < ActiveRecord::Base
  has_state :state, 
    :with_states => [:active :expired]
  delegates :expired?, :refresh!, :to => :state, :with_self => true
end

and a good deal of the ugliness goes away.

The same thing applies to your Publication example, except there you’re dealing with a Strategy (it’s the same as a state really, but it doesn’t change once you’ve made the object). Instead of using a disguised if (not that disguised when you think about it), you rely on polymorphism to do the dirty work for you.

Note the complete lack of if statements in this solution, whether in the visible code or the stuff behind the library curtain.

Floyd

May 01

Floyd said,

Kingdom of Nouns

Piers Cawley

May 01

Piers Cawley said,

Oh, please Floyd. If you’re going to attempt a refutation by href, at least make sure the document at the end of the link has some bearing on the discussion. Stevey makes an excellent point and all, but it has absolutely nothing to do with this.

I’m just applying the “replace conditional with polymorphism” refactoring. No verbs were nouned in the course of the refactoring, I just introduced a couple of new nouns that verb differently, that’s all.

I admit that I have, in my time, replaced a verb with a noun, but generally that’s been a step on the way to breaking a nasty compound verb up into a bunch of smaller, more comprehensible verbs.

Floyd

May 01

Floyd said,

Piers, I didn’t mean to appear flippant; I think an href without comment should be taken as it stands. That said, your StateBuilder class in particular merits reference to Yegge’s argument.

Jon Yurek

May 01

Jon Yurek said,

No, sorry, Jarred, but that is not an example of “tell, don’t ask”. You’re still differentiating behavior in the same place, you’ve just made your asking a little more complex. Instead of asking if the subscription is expired, you’re asking for all the nonexpired and then all the expired ones. If you really wanted to adhere to “tell, don’t ask” you wouldn’t have a method on Subscription doing any differentiating at all. You’d simply 
1
2
3
4
5
6
7
8
9

class Subscription
  def self.update_subscriptions
    self.find(:all).each(&:refresh)
  end

  def refresh
    # Do the conditional here
  end
end
because that way you’ve abstracted the renewing into the instance. You’ve told the object to refresh its subscription and you don’t care about the rest. Once you’re in the object, handling your own behavior, there’s no one left to tell, you just do. I’m firmly against any principle or pattern if it stands starkly in the way of getting something done.

It’s even worse with your before-create methods. You’re not getting rid of your if statement. You can’t claim you are when your before-create methods have “if” right in them. If you are differentiating that way, you’d be better off having subclasses.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

class Publication < ActiveRecord::Base
  before_create :gateway_find
  def gateway_find
    # do nothing here. Maybe raise?
  end
end

class ABCPublication < Publication
  def gateway_find
    self.attributes = AbcGateway.find name
  end
end

class XYZPublication < Publication
  def gateway_find
    self.attributes = XyzGateway.find name
  end
end

They’re similar things that differentiate on behavior, right? They belong in different classes.

I don’t understand what all the hate is about if statements. You railed against them the other day because you didn’t like them, and tried to replace them with complicated state machinery, but fell back to them because they were the most logical choice. Yes, if your states grow it may be simpler to look to a state machine or more complicated machinery that can scale with the problem, but you did come around to the answer in the end.

We need to keep the simple things simple and make the hard things look simple. Loop-based conditionals make the simple things look hard.

Brad Ediger

May 01

Brad Ediger said,

I’d rewrite that first method, using Symbol#to_proc, as:

def update_subscriptions
  subscriptions = Subscription.find :all
  subscriptions.select(&:expired?).each(&:renew!)
  subscriptions.reject(&:expired?).each(&:update!)
end

That seems more direct to me, and when you take out the punctuation it almost reads like English. I tend to prefer this style.

@labrat: IIRC, naming the instances “each” is a Smalltalk thing. Not too common in Ruby, and I agree it is a tad confusing here.

Jon Yurek

May 01

Jon Yurek said,

If you’re going to do the select/reject route at all, you should at least use #partition. Defiant versus conditionals or not, there’s no reason to run the loop twice.

Piers Cawley

May 01

Piers Cawley said,

Brad: Having been convinced by by Kent Beck’s arguments for calling the parameter of a do: block each, I’ve standardized on value in my Ruby iterators. inject blocks get |initial, item| parameters and iterators over hashes get a key as well.

Floyd: Yeah, StateBuilder is a smell arising from the workings of composed_of. The as yet unwritten has_state can just call a method. (composed_of would be so much nicer if you had the option of giving it a block to build the value object).

Brad Ediger

May 01

Brad Ediger said,

Jon: Much agreed. I thought about changing the code to use #partition here, but I was aiming for a somewhat direct translation and didn’t want to change too many things at once.

Piers: I tend towards x or even it for each and map. My inject blocks are context-dependent: |sum, value| or |hash, x| if I’m building a hash.

Usually I’m just lazy and x marks the spot. To paraphrase Larry Wall, these are pronouns, so short names are preferable to descriptive ones.

Piers Cawley

May 01

Piers Cawley said,

I went with value because it sits well with key.

It’s one of those areas where what is important is not what you choose, but that you choose, and you stick to your choice.

Brad Ediger

May 01

Brad Ediger said,

Jon: I just realized you mentioned partition earlier, in your first comment. Serves me right for not reading before posting!

Piers Cawley

May 01

Piers Cawley said,

Gah, meant to add that I don’t use x or i because x is an unknown real and i is an integer index.

Peter Cooper

May 01

Peter Cooper said,

Sadly I don’t really get the jist of the first example at all. The rewrite seems a lot harder to read. I’d personally also ditch the if, but keep it like this:

def update_subscriptions Subscription.find(:all).each do |subscription| subscription.expired? ? subscription.renew! : subscription.update! end end
jare care

May 02

jare care said,

john,

Yes it is true I ‘asked’ an Array object for its elements and then told its elements what to do. But collections are an exception to “tell don’t ask” because their most fundamental behavior is adding and removing elements. I wouldn’t want to re-open Array and put my method in there just to avoid asking the collection for its elements.

Yes, putting the method on the class side of Subscription would allow me to just ‘tell’ the Subscription class object what to do but you’d still need the conditional logic in your #refresh instance method. I avoided that conditional logic using #select and #reject; taking advantage of Array’s filtering behavior.

In the other example, you’re right in that subclassing Publication would avoid the need for an ‘if’ parameter to the #before_create class method. I thought that was overkill in my example and to me an ‘if’ parameter is not the same as an ‘if’ statement so I’m fine with it.

‘each’ is from Beck. I like it because its simple and it brings consistency to your code. I don’t like 1 letter enumeration parameters or naming it after the class e.g. |subscription| because one is not descriptive enough and the other too descriptive. ‘each’ is nice because it sits right between the 2 extremes.

Also, I’m not a huge fan of the new Rails’ addition of Symbol#to_proc, i think the block is more elegant, better looking and requires less Ruby knowledge so more people can understand it than people.collect(&:name).

Piers, I like how you stored the state class name in the db and #constantize’d it; very nice.

Jon Yurek

May 02

Jon Yurek said,

Collections are not an exception. That’s why things like each and inject exist, to tell the array to do something for each item it contains. That’s beside the fact that you’re asking for the different collections.

I think this is where we’re not seeing eye-to-eye. Asking for the expired and unexpired collections is a conditional branch, just wrapped in fancier terminology. There is a conditional branch inherent in the select and reject. I can rewrite the implementation of both right now, and they are nothing if not a simple conditional.

to me an ‘if’ parameter is not the same as an ‘if’ statement

This is why I disagree with the general content of this post. You are using a conditional, but as long as it looks different to you, you’re ok with it. There’s something about if statements specifically that you dislike, and I have no idea what it is, but the fact that you’re ok with a longer (because it’s a proc), differently defined (because it’s a callback) if statement, and you’re ok with more complex (but if-less) code, but you’re not ok with having a simple if (when it’s the simplest means of obtaining the correct answer) means that for whatever code smell you’re trying to get rid of, you’re attacking the symptoms and not the cause.

On the other subject, “each” is as (if not more) meaningless as a single letter, plus it’s more typing. But that’s a different discussion.

Tom Armitage

May 02

Tom Armitage said,

...and it’s one that’s already happened around these parts (with much the same opinions espoused).

To stay briefly off-topic: I’m not sure how “subscription” is too descriptive an iterated variable name. It’s rare that anything’s ever too descriptive, surely?

Jeremy Weiskotten

May 02

Jeremy Weiskotten said,

I’m with Piers. If you really want to refactor away lots of ugly conditional logic, polymorphism is often a good way to do it. Josh Kerievsky documents this in “Refactoring to Patterns” as “Replace Conditional with Polymorphism”.

However, it can be a relatively heavyweight solution, so it might not be worth the effort if you’re looking at just an if/else or two. It’s most valuable when you have methods that contain if/elsif/elsif/elsif… conditions, especially in multiple places.

Ed (courtesy of Greg)

May 02

Ed (courtesy of Greg) said,

Subscription.find(:all).partition(&:expired?).zip([:renew!, :update!]) {|c,m| c.each(&m) } # FTW!

Piers Cawley

May 08

Piers Cawley said,

Futher to this, I’ve just implemented has_state for typo. There’s more on my blog if you’re interested.

Sorry, comments are closed for this article.

© 2000 - 2009 by thoughtbot, inc.
written by a bushel of tiny robots