Welcome to Giant Robots Smashing Into Other Giant Robots — a weblog about development, business, design and technology — written by thoughtbot.

designing without ifs

Alright here we go.

Say we got an app with groups, users and memberships.

Theres 2 different types of groups:

Public
Private

Now anyone can join a public group but in order to join a private group we have to authenticate you against an external api. For example, say there’s a private group called ‘Yahoo’ and in order to join it you have to have a Yahoo account (yes this is nuts but hear me out).

I’m going to factor out the external api code into a library and use ActiveRecord callbacks to perform the validation.

Let’s take a look at this.


module PrivateGroupApi

  class Request

    attr_accessor :url

    def initialize(url)
      self.url = url
    end

    def authenticate(username, password)
      uri = URI.parse "#{url}?username=#{username}&password=#{password}"
      request = Net::HTTP::Get.new uri.path
      http = Net::HTTP.new uri.host, uri.port
      response = http.request request
      PrivateGroupApi::Response.new response.code
    end

  end

  class Response

    attr_accessor :code

    def initialize(code)
      self.code = code
    end

    def success?
      code == '200'
    end

    def failure?
      code == '404'
    end

    def error?
      code == '500'
    end

  end

end

Looks straightforward, a Request and a Response class. The Request class takes a url during construction and its #authenticate method takes a username and password that will be looked up by the external api. We’ll make a requirement saying a 200 is a valid account and a 404 is an invalid account.

Now for some usage.


class Membership < ActiveRecord::Base

  belongs_to :group
  belongs_to :user

  validate_on_create :external_membership?

  attr_accessor :username,
    :password

 private

  def external_membership?
    if private?
      request = PrivateGroupApi::Request.new group.api
      response = request.authenticate username, password
      if response.failure?
        errors.add_to_base "You do not have an account with this group's web site"
      end
      if response.error?
        errors.add_to_base 'There was a problem when trying to authenticate your account'
      end
    end
  end

end

Ugh, 2 conditionals; testing for failure and testing for errors (network, api errors, etc.). Conditional logic breeds bugs and always complicates things, I prefer for libraries and frameworks to do the decision making for me.

Let’s rewrite.


module PrivateGroupApi

  class Request

    attr_accessor :url,
      :username,
      :password,
      :listeners

    def initialize(url)
      self.url = url
      self.listners = {
        '200' => lambda {},
        '404' => lambda {},
        '500' => lambda {}
      }
    end

    def authenticate
      uri = URI.parse "#{url}?username=#{username}&password=#{password}"
      request = Net::HTTP::Get.new uri.path
      http = Net::HTTP.new uri.host, uri.port
      response = http.request request
      listener = listeners[response.code]
      listener.call
    end

    def on_failure(&block)
      listners['404'] = block
    end

    def on_error(&block)
      listners['500'] = block
    end

  end

end

This redesign eliminated the need for a Response class. Instead it uses an event driven style with listeners that will be notified when a particular response is given.

Now some usage again.


class Membership < ActiveRecord::Base

  belongs_to :group
  belongs_to :user

  validate_on_create :external_membership?

  attr_accessor :username,
    :password

 private

  def external_membership?
    if private?
      request = PrivateGroupApi::Request.new group.api
      request.username = username
      request.password = password
      request.on_failure do
        errors.add_to_base "You do not have an account with this group's web site"
      end
      request.on_error do
        errors.add_to_base 'There was a problem when trying to authenticate your account'
      end 
      request.authenticate
    end
  end

end

Thats much better. We eliminated both conditionals by giving the library the code to run when a certain response is given. No more requiring users to make decisions based on responses, resulting in much easier to read and less error prone code.

That still leaves us with 1 conditional, which I don’t care for, in the GroupMembership model. Look at the declaration of the #external_membership? validation callback

12	validate_on_create :external_membership?

That is poor because to someone reading the code it seems like this validation applies to all groups. Only when you read the implementation of #external_membership? do you see a conditional in there applying this validation only to private group types.

Rails needs to give us the following:


class Membership < ActiveRecord::Base

  belongs_to :group
  belongs_to :user

  validate_on_create :external_membership?,
    :if => lambda {|membership| membership.group.private?}

  attr_accessor :username,
    :password

 private

  def external_membership?
    request = PrivateGroupApi::Request.new group.api
    request.username = username
    request.password = password
    request.on_failure do
      errors.add_to_base "You do not have an account with this group's web site"
    end
    request.on_error do
      errors.add_to_base 'There was a problem when trying to authenticate your account'
    end
    request.authenticate
  end

end

And with that we eliminate the last conditional and the validation declaration reads much more accurately “if this is a membership for a private group, is its username and password registered with its group’s web site”. Rails has taken care of the decision of whether to apply the validation or not.

I like this event driven style of design. Web frameworks give us this when handling requests e.g. in Rails your routes are mapped to controllers and individual methods within a controller. The framework is doing the decision making for you when a particular request comes in.

About this entry

You're reading an entry on GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS, the company weblog of thoughtbot, inc.

Author:: Jared Carroll

Published:: January 1st 10:07 PM

Updated:: January 1st 10:54 PM

Sections:

thoughtbot is hiring

We are hiring web developers and web designers in both Boston and New York, NY.

What are we up to?

We built Shoulda, an eclectic set of additions to Test::Unit; Paperclip to manage uploaded files without hassle; Jester, a REST/ActiveResource client library written in Javascript, and Squirrel, an enhancement for ActiveRecord's find syntax; — amongst some other projects.

Chad (President) and Jon (CTO) co-authored a technical book titled Pro Active Record: Databases with Ruby and Rails, which explores the ins and outs of the ActiveRecord ruby library. You can buy it today at Amazon.com.

About thoughtbot, inc.

We are a small web application development consulting business, with offices in Boston, MA and New York, NY. If you're looking to find a team for your next web development project or your new web application — get in touch.

Justin Blake 01.02.08 / 10AM

I just love watching ifs disappear. Good times.
Jon Yurek 01.02.08 / 10AM

Man, this post has me all conflicted. On the one hand, I like the callbacks, but on the other hand, you’re not really clarifying the code. “request.on_failure do” isn’t any more clear than “if request.failure?”. You still have three separate code paths. Did you really simplify this?

The :if in the validate_on_create is good. Even though I think it’s funny that you “got rid” of an if by moving it from one function to another.
Tammer Saleh 01.02.08 / 01PM

I think I agree with Jon. The use of callbacks is cleaver, but not nearly as linear and easy to debug as the conditional version. I definitely like the use of the :if modifier in the validation statement, though. Makes that much more legible.

Florian Aßmann 01.02.08 / 07PM

I put some braincells of mine to work this out but since I don’t have a running ruby environment here It’s 100% theory:


class Group < ActiveRecord::Base
  has_many :memberships
  has_many :users, :through => :memberships

  def acceptance(user, &block)
    block.call Status.new(200)
  end

  class Status
    CODES = {200 => :ok, 404 => :not_found, 500 => :internal_server_error}
    CODES.values.each do |method_sym|
      define_method(method_sym) {}
    end

    def __active(&block) block.call end

    def initialize(code)
      (class << self; self; end).module_eval do
        alias_method CODES[code.to_i], :__active
      end
    end

  end
end

class PrivateGroup < Group
  def acceptance(user, &block)
    uri     = URI.parse "#{url}?username=#{user.username}&password=#{user.password}" 
    request = Net::HTTP::Get.new uri.path
    http    = Net::HTTP.new uri.host, uri.port

    block.call Status.new(http.request(request).code)
  end
end

class Membership < ActiveRecord::Base
  belongs_to :group
  belongs_to :user

  validate_on_create :acceptance_of_group

  def group_acceptance
    group.acceptance(user) do |state|
      state.not_found { errors.add_to_base 'You do not have an account with this group\'s web site' }
      state.internal_server_error { errors.add_to_base 'There was a problem when trying to authenticate your account' }
    end
  end

end

I love STI even though Rails support for it does still suck sometimes or almost everytime but it kicked out one more if-clause…

Cheers Florian

Today / Now

GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS

designing without ifs

About this entry

thoughtbot is hiring

What are we up to?

About thoughtbot, inc.

4 comments

Recently

Categories

The thoughtbotsphere