Close
Glad You're Ready. Let's Get Started!

Let us know how we can contact you.

Thank you!

We'll respond shortly.

LABS
Object Oriented Rails – Writing better controllers

I have been doing a lot of mobile development lately, using Objective-C on iOS, and Java on Android. Since I’m also a Ruby developer, it makes a lot of sense for me to try and apply what I learn from each of these languages and frameworks whenever I can.

Today I’m going to do my best in writing better controllers in my Rails app. When writing an app for Android, or iOS, I force myself not to use stubs or mocks, but instead I try to improve the object architecture and use dependency injection to write my testing code.

Let’s take an example of code:

class User
  validates_uniqueness_of :username
end
class RegistrationController < ApplicationController
  def create
    user = User.where(username: params[:username]).first
    user ||= User.new.tap do |new_user|
      new_user.username = params[:username]
      new_user.save!
    end
    render json: user
  end
end

Here we only need a username to register the user, and if the user is already registered we just return it. Very simple. But that logic does not belong to a controller, and it would be impossible to test it without connecting to that database and without using a pretty big mess of stubs and mocks. A good indicator of “you’re doing it wrong” would be having: User.any_instance in your specs.

A first improvement could be the following:

class User
  validates_uniqueness_of :username

  def self.register(username)
    user = User.where(username: username).first
    user ||= User.new.tap do |new_user|
      new_user.username = username
      new_user.save!
    end
  end
end
class RegistrationController < ApplicationController
  def create
    user = User.register(params[:username])
    render json: user
  end
end

This is already much better code, you only need to stub User.register to be able to test that controller. And that is just fine.

Now you could just stop there, and if your app does not grow too much you can handle it pretty nicely. But if your app starts growing, your User model will grow bigger and bigger, you will start giving it more responsibility, more than just knowing: “Am I valid?”.

A better way of splitting that and actually getting rid of stubs would be that final piece of code:

class User
  validate_uniqueness_of :username
end
class RegistrationService
  def register(username)
    user = User.where(username: username).first
    user ||= User.new.tap do |new_user|
      new_user.username = username
      new_user.save!
    end
  end
end
class RegistrationController < ApplicationController
  before_filter :load_registration_service

  def create
    user = @registration_service.register(params[:username])
    render json: user
  end

  def load_registration_service(service = RegistrationService.new)
    @registration_service ||= service
  end
end

The registration service will only be responsible for registration related database access. Note that this service could be switched to another one that actually would use an Http service instead of the database. Also now you can entirely get rid of stubs in your tests, as follows:

describe RegistrationController do
  before do
    @fake_registration_service = controller.
        load_registration_service(FakeRegistrationService.new)
  end

  describe "POST #create" do
    it "delegates to the registration service and renders the returned user" do
      expected_user = User.new.tap {|u| u.username = "damien"}
      @fake_registration_service.registered_user = expected_user

      post :create, username: "damien"

      expect(response.status).to eq(200)
      expect(response.body).to eq(expected_user.to_json)
      expect(@fake_registration_service.username_registered).to eq("damien")
    end
  end
end
class FakeRegistrationService
  attr_accessor :registered_user
  attr_reader :username_registered

  def register(username)
    @username_registered = username
    registered_user
  end
end

Sadly, we don’t control in Rails how our controllers get instantiated. So we need to use something like that trick with the before filter using a default value for our service. With a PORO you would define a constructor that does something similar.

Dependency injection is very common when test driving Android apps and iOS apps. I personally like to never use mocks on these platforms. And I’m trying to do more of it in Ruby on Rails applications. It will force you to think your object architecture better, having more dedicated objects, limiting logic in value objects (like models and presenters)…

Let me know what you think of this! :)

Comments
  1. Winston Teo says:

    I could be missing something.. But I would write the controller as:

    # Rails 3
    User.find_or_create_by_username(..)

    # Rails 4
    User.where(..).first_or_create

    What do you think?

  2. Andrew Bruce says:

    I don’t understand how exposing a writer method on a controller helps you get rid of mock or stub objects, nor how this is an argument for getting rid of mocks and stubs.

    FakeRegistrationService is a custom-rolled test double that serves a similar purpose to a mock, but doesn’t expose the message sent to the object in the test. I’d rather have a ‘dumber’ generic mock object that exposes and restricts messages sent to it rather than one I have to go and read the source to understand.

    The other concern I have is with adding extra code to the application that is never exercised in the real world. I prefer to submit to Rails and let it force me into integration testing my controllers. I’ll use mocks further down the stack, at the unit level, to drive out separation of concerns and all that lovely stuff. The controller is where I’ll compose concrete objects that play the roles I mock out in unit tests.

    So:

    * why is FakeRegistrationService better than a pure mock object?
    * why is adding a writer method to the controller better than ‘wild’ stubbing on a concrete model’s class method or a controller’s reader method (the latter perhaps being more forgiving of change)?
    * why test controllers with test doubles at all?

  3. Damien Le Berrigaud says:

    * why is FakeRegistrationService better than a pure mock object?

    It forces you to think more of a proper interface for your object. While having the ability to stub/mock anything will not push you towards that train of thoughts.

    * why is adding a writer method to the controller better than ‘wild’ stubbing on a concrete model’s class method or a controller’s reader method (the latter perhaps being more forgiving of change)?

    Well if Rails allowed me to instantiate or inject dependencies in my controllers (i.e. control the lifecycle of my controllers), I would do it differently. Dependencies are meant to be injected in the constructor. The before_filter is just a way to make it works with a Rails controller.

    Then again the exercise here is to try and think without stubs, the easy (sometimes dirty) solution.

    * why test controllers with test doubles at all?

    This is a unit test, this is not integration testing. This should not depend on the database. Using the database is slow. Doing correct continuous integration requires a fast test suite.

    Let me know if I misunderstood your questions :)

  4. Damien Le Berrigaud says:

    @Winston Teo

    That would work yes. Here I stripped the example down, but there could be much more code around the user registration logic obviously :)

  5. Erik Peterson says:

    I suspect Andrew’s point about integration testing is: Why even bother unit testing controllers? A unit test like this does not supplant the need for a full-stack integration test elsewhere. If you have such an integration test, I believe that proves that the code works, and makes the unit test superfluous.

    I generally advocate for 100% unit test coverage, but Rails controllers are kind of a special case in that they’re (if you’re doing it right) extraordinary simple chunks of code who are very difficult to unit test. Relative to other code, this increases the cost of testing while decreasing the value, and in my opinion makes unit testing not worth it.

  6. Mike Gehard says:

    I’m concerned about the message that one might get from this article: “mocking/stubbing is bad” and that you should go to more complex solutions to inject dependencies just for your tests. Don’t give DHH another reason to call “pattern wankery”. :-)

    I agree that the use of service objects is a great way to unit test your Rails controllers, especially when the logic in them gets complex. I also love the idea of injecting dependencies to keep lines of separation clear.

    What I disagree with is adding a public method to the controller interface just so that you can call it in your tests to inject the service dependency. Why not just use something like (in RSpec):

    RegistrationService.stub(:new).and_return(@fake_registration_service)

    You are stubbing a very stable interface in Ruby so you don’t have to worry about your stubs getting out of line with implementation without adding code to the controller to satisfy a need of your tests.

    I’m not convinced that the use of a fake service is the best way to go over a mock object but in a small example like this one it is hard to tell if the fake service soothes some pain point.

    Mocks/stubs have been around a long time and have been used with great success. Don’t throw the baby out with the bath water just because mocking/stubbing in Ruby can be a dangerous endeavor. Like everything else, moderation is the key.

    Hopefully you have request level tests that will catch when you have done something wrong with your mocks/stubs.

  7. Damien Le Berrigaud says:

    I agree that it would be just fine in that example to just use a mock instead of a fake here.

    As I mentioned in my previous comment though, using a fake is an interesting exercise that forces you to think: “what if the language did not allow me to mock”.

    Also it makes the test a bit more readable in my opinion, but then again using a mock at that point is just fine.

  8. Xavier Shay says:

    I’ve been experimenting with a way to write PORO controllers and have been finding it really nice. It is making my controllers easier to read, refactor, and test.

    https://github.com/xaviershay/poniard

    AFAIK no one else is using this yet, but would love to hear of other experiences :)

  9. Guy Maliar says:

    How about having RegistrationService as a singleton? That way the before_filter can be removed and instead you’d only initialize the service when it is needed?

  10. Jared Carroll says:

    To me the point of this post isn’t about controller testing. It’s about keeping
    responsibilities separate. User registration isn’t a part of an application’s
    domain; it’s a by-product of modeling a domain in software, specifically a web
    app. If you don’t feel controller tests like these add value, then I would skip them and
    instead rely on a higher-level integration test for coverage.

  11. chrismo says:

    First, let’s be clear on the distinction between mocks as a concept and a mocking framework. You can use one without the other.

    Relying too much on magic in a mocking framework is a code smell to me. It’s convenient, but it may be telling you there’s a DIP violation in your code.

    I’d add to that that while simple uses of mocha and such are quite convenient, I’ve had problems on the other end when things start blowing up and had difficulty getting to the root of the problem because I’m wading through so much magick codez.

  12. Cade says:

    I may have misread/misunderstood, but I felt like there was some bait and switch in this article.

    You start out by introducing some registration code in the controller and present a clear problem:

    “…it would be impossible to test it without connecting to that database and without using a pretty big mess of stubs and mocks. A good indicator of “you’re doing it wrong” would be having: User.any_instance in your specs.”

    You then proceed to move the registration code in question to a service object and demonstrate how using a fake test object and dependency injection can “entirely get rid of stubs in your tests” (aka the indicator that “you’re doing it wrong”).

    But you haven’t tested the original registration code. At all.

    You just moved the “hard to test” code into another class and didn’t show us the specs for that class. Are you connecting to the database or using stubs and mocks to test your RegistrationService class?

    I understand there are other benefits to structuring your code this way (which you mention to a certain extent: “Note that this service could be switched to another one that actually would use an Http service instead of the database.”), but the aim of the code samples seems primarily framed around testing benefits, which seems dubious without actually providing tests for the original registration code in question.

    Am I missing something?

  13. Damien Le Berrigaud says:

    @Cade

    Yes, in most cases I would simply test the actual DB behavior in my service. Also, if instead I’m going to use an Http API instead of the database (which is the case in mobile apps most of the time), then I will use a Fake Http layer to test my service.

    It remains important that your controller doesn’t deal with active record or whatever else you are using as your data layer. Best solution is that which ever you are using in your service, you can stub it as well using a fake or simple mock too.

    There always is a balance to find between using mocks and fakes, but you definitely want to separate the controller layer from the data layer as much as possible.

    Let me know if you’d like more details.

  14. I’m embarrassed to say that I’ve experienced the pains of not delegating responsibilities properly. Because of that, I agree with the reasoning to create the RegistrationService. This approach has allowed me to decouple my controller tests from implementation details, and to use way less stubs and keep my tests useful, fast and with minimal setup.

    Even though you’re not using mocks/stubs per se, it seems to me like the testing approach you’re trying has pretty much the same negative side effects.

  15. Keenan Brock says:

    One tangental thought is to use nouns as service names like you do with models. So calling it a user registration. Sorry I don’t have a link to the original author on the concept.

    So you create a user registration which does all the stuff in your service layer. Testing that is straight forward and when time comes to expand, or serialized to db you will be all set.

  16. […] Object Oriented Rails: Writing Better Controllers Damien Le Berrigaud of Pivotal Labs tries to avoid stubs and mocks and leans on dependency injection to test his controllers' code. […]

  17. Hao Liu says:

    @Andrew, you are saying we should do controller tests as integration tests as much as we can, and absolutely not introduce any mock/stub in the controller tests, it’s high level tests than the unit tests with which we can capture the interface changes underneath.

    If it’s yes, it does clarify the doubt i always have, since i do believe “functional” tests are “integration” tests, why people split them into two folders “functions” and “integrations”.

  18. Matthew Rudy says:

    @Winston Teo

    I have a policy not to use ActiveRecord syntax outside of the model.
    I also have a policy of never stubbing code on a model.

    If you need to stub it, then it doesn’t belong on the model.

    Saying that `find_or_create_by_username` is actually the perfect method for this use-case. So I would just use it.

  19. Great post. After you moved the complex part into a service, then you could do insolation testing here so nice, but how about testing in the service object, so do you test it in insolation as well? Because I see in the example it has coupling like AR’s query interface that could drive to have a complex setup stub/mock in the testing code.

  20. Alex says:

    This is very debatable article.

    First of all, wtf is

    > user = User.where(username: params[:username]).first

    I think, better will be User.find_by_username

    Secondly, do you know about something like first_or_create?

    And, RegistrationService is unnecessary, I think

    • Damien Le Berrigaud says:

      It seems many people don’t get the point of this post. This is just to illustrate a way of creating controllers that are not tightly coupled to active record.

      The code above is just an *example*, this is not a real app, and it’s not there to show off knowledge of Active Record’s syntactic sugar.

      Hopefully one day you will write an app big enough, and you will ask yourself the same questions I’m trying to make you reflect on, in this post.

  21. […] me feeling a little isolated when I can’t keep up with intense conversation on the merits of refactoring controllers with the additions of a service layer, for […]

  22. prashanth says:

    Hi Damien,
    Would you use services for simple queries like @member.update(params) or filtering ?

    I have used services but unsure how much logic should be placed in it. Like after a user is registered a email needs to be sent in background. Would this be placed in RegistrationService like redis.enquque(‘send_email) or should it be part of controller.

Post a Comment

Your Information (Name required. Email address will not be displayed with comment.)

* Copy This Password *

* Type Or Paste Password Here *