designing without ifs

Posted by jcarroll

Jan 01

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.

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
38
39
40
41
42
43
44


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.

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


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.

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
38
39


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.

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


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

1
2


  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:

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


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.

Comments on this post

Justin Blake

Jan 02

Justin Blake said,

I just love watching ifs disappear. Good times.

Jon Yurek

Jan 02

Jon Yurek said,

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

Jan 02

Tammer Saleh said,

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

Jan 02

Florian Aßmann said,

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

Sorry, comments are closed for this article.

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