designing without ifs

Alright here we go.

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

Theres 2 different types of groups:

  1. Public
  2. 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

  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.

Jared Carroll

Hound automatically reviews Ruby, JavaScript, and CoffeeScript code in your GitHub pull requests and comments on style violations. It is free for open source repos and $12/month per private repo.