October 3, 2011
In the last few months I’ve been learning a lot about object-oriented design. By following the conversations and writings of Avdi Grimm, Corey Haines, Steve Klabnik, Gary Bernhardt and others (even going way back to James Golick), I’ve been able to identify some design issues in the codebase I work on every day at Paperless Post. These are issues that I had sensed previously, but had lacked a vocabulary with which to discuss them. In this post I consider a refactoring I undertook recently in our codebase. Through this discussion, I hope to help spread the good word on design in Rails applications.
The code I’m working with here is for Paperless Post’s integration with Eventbrite. Recently, we launched a feature that allows users to connect their account with an Eventbrite account. This allows users to link invitations on our site with ticketed events on Eventbrite. As a new requirement emerged, my original implementation became inadequate.
The Old Code
The following controller is responsible for connecting a user’s account with their account on Eventbrite:
class EventbriteAccountConnectionsController < ApplicationController def create if connection = EventbriteAccountConnection.connect( :account_id => current_account.id, :email => params[:email], :password => params[:password] ) client_messenger.add_notice "Successfully logged in to Eventbrite." client_messenger[:connection_id] = connection.id else client_messenger.add_error "Sorry, we were unable to log you in to Eventbrite." end send_messages end end
There are two details to note here. One is that
send_messages are methods that deal with a
ClientMessenger, an object for preparing and sending JSON responses to the client. I’ll use this again later on. The other detail is that when connecting accounts is successful, the client needs the ID of the connection record, so the controller must include this data in the response.
This is the model code that is called by the controller:
class EventbriteAccountConnection < ActiveRecord::Base def self.connect(credentials) resp = EventbriteClient.user_get(credentials[:email], credentials[:password]) return if resp['error'] self.create( :account_id => credentials[:account_id], :code => resp['user']['user_key']), :remote_id => resp['user']['user_id'] ) end end
This code attempts to fetch a user from Eventbrite’s API and, if successful, creates and returns a connection record. If the API request responds with an error, the method returns
nil. This code worked reliably, but eventually our team wanted to convey error messages from Eventbrite to users rather than just indicating success or failure. This way we would be able to distinguish between a failure with the user’s Eventbrite account, or a mistake on our site.
The most immediate solution to this problem would have been to modify the model method to return a more descriptive response in the case of failure. Then the controller could receive the error message and pass it along to users. This means that in case of success, the method would return an
EventbriteAccountConnection whereas in failure it would return a
String. Maybe it’s the Haskell talking, but I tend to think of wildly differing return types as a code smell. If the types respond to the same messages, it’s all good: duck-typing should be embraced in our code. But in this case there would be little recourse other than to check the type of the response: if the controller gets back a
String, there was an error; if it gets an
EventbriteAccountConnection, success. Ick.
A better option would be to for
EventbriteAccountConnection.connect to return a
Struct subclass that has a slot for an error message and a slot for an
EventbriteAccountConnection instance. If the connection instance is present, that indicates success. If it’s missing, the login failed, and we should display the struct’s error attribute.
This could be a workable solution, but it sidesteps some fundamental design problems in
EventbriteAccountConnection.connect. The most obvious issue to me was that it is a class method. I don’t think, as some do, that class methods are evil. However, they can indicate an attempt to use a class as a “bucket of methods,” ignoring or avoiding the advantages of encapsulating data and behavior in an object. In this case, I defined this as a class method because I could sense that the ActiveRecord model couldn’t provide the encapsulation I needed. But at the time, I didn’t take the further step of defining a new class to abstract out the connection logic.
The New Code
Given the additional requirement of improved error propagation, I took the opportunity to extract this account connection logic into a new domain object. Here is the code I arrived at, with some details omitted:
class EventbriteAccountConnection::ConnectionAttempt def initialize(account, email_address, password) @account = account @response = EventbriteClient.user_get(email_address, password) @connection = save_connection end def as_messenger(messenger = ClientMessenger.new) if success? messenger << "Successfully logged in to Eventbrite." messenger[:connection_id] = @connection.id else messenger.add_error(error_message) end messenger end end
This my not be the greatest Ruby class ever, but it’s a significant improvement in design. To use the terminology of Hal Abelson and Gerald Sussman in Structure and Interpretation of Computer Programs, I have created a data abstraction, binding together an account and some login information into a bundle that has some meaning. By introducing the name
ConnectionAttempt I have brought to the surface – I have reified – a concept that was previously submerged. This will aid future readers and maintainers of this code. After all, the goal of code is not just to provide instructions for the computer to carry out – it is also to communicate meaning to other humans. In this sense, this refactoring is a success.
The new service object also yields a simpler controller:
class EventbriteAccountConnectionsController < ApplicationController def create render :json => EventbriteAccountConnection::ConnectionAttempt.new( current_account, params[:email], params[:password]).as_messenger end end
This is my ideal controller: it contains no logic. It merely calls out to a collaborator, passing it some parameters, and returns a response. While, in many cases, building a response directly in the controller is appropriate, in this case, given the non-trivial logic for generating the response, I’ve made this the responsibility of
ConnectionAttempt. This prevents the controller from having to reach into its collaborator to query its internal state. It tells the object what it wants rather than asking.
The entirety of the ActiveRecord model is now:
class EventbriteAccountConnection < ActiveRecord::Base end
This svelte class now respects the Single Responsibility Principle. Its sole purpose is to persist account connections to the database.
ConnectionAttempt has a narrow interface to
EventbriteAccountConnection, it’s easy to mock out this external dependency. This makes the tests faster since it eliminates the need to persist ActiveRecord objects to the database. It also allows running the test without loading the entire Rails application. But there’s an additional, more subtle advantage to this use of mocking. By isolating
ConnectionAttempt from the
EventbriteAccountConnection that it ultimately attempts to create, the test becomes clearer because it takes advantage the abstraction barrier between the two domain concepts, allowing the test to focus more strictly on the object under test.
It’s a bit silly how revelatory this feels to me. Object-oriented design has never been kept a secret, yet it seems this knowledge has been forgotten in the context of Rails applications. At the same time, to some to some, this design knowledge has been a given for years. In my case, although I read James Golick’s post on the topic almost two years ago, when I was starting to use Ruby and Rails, I didn’t piece it all together until recently. Now I know that by paying attention to design, we can build more maintainable, more understandable Rails apps, with faster test suites.
I’d like to hear from you. What do you think of this code? How would you continue this refactoring? What design strategies have you applied in your Rails apps, and what lessons have you learned?