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.
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.
