designing without ifs

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 Developer

Hound reviews Ruby and CoffeeScript code for style violations and comments on your GitHub pull requests. Free for open source repos.