This has a couple of significant plus points: First, no instance variables, so both the controller action and the view are more readable and easier to refactor. Second, no instance variables! So there’s a clear, documented, named interface between the controller and the view. And third, this is so transparently readable that I never bother to test it.

The wiki used in the above action is a repository, built in a memoizing helper method that most of the controllers use:

In this case the correct kind of repository is created for the current user, and all of the other code in this request sits on top of that. So the controller action, helped by the memoized repository builder, effectively constructs an entire hexagonal architecture for each request, and the domain logic is thus blissfully unaware of its context.

Here’s a slightly bigger example. This is for a page that shows a variety of informative graphs about the wiki; and because I may want to re-organise my admin pages in the future, each graph’s data is built independently of the others:

That’s the most complex query controller action I have, and I maintain that it’s so simple I don’t need to test it. Would you?

The Longer Story
Shorter methods protect my investment in the code, in a number of ways.

First: Testability. I expect there to be a correlation between method length (number of “statements”) and the number of (unit) test cases required to validate the method completely. In fact, as method size increases I expect the number of test cases to increase faster than linearly, in general. Thus, breaking a large method into smaller independent methods means I can test parts of my application’s behaviour independently of each other, and probably with fewer tests. All of which means I will have to invest less in creating and running the tests for a group of small methods, compared to the investment required in a large method.

Second: Cohesion. I expect there to be a strong correlation between method length and the number of unknown future requirements demanding that method to change. Thus, the investment I make in developing and testing a small method will be repaid countless times, because I will have fewer reasons in the future to break it open and undo what I did. Smaller methods are more likely to “stabilize out” and drop out of the application’s overall churn.

Third: Coupling (DRY). I expect that longer methods are more likely to duplicate the knowledge or logic found elsewhere in the same codebase. Which means, again, that the effort I invest in getting this (long) method correct could be lost if ever any of those duplicated pieces of knowledge has to change.

And finally: Understandability. A method that does very little is likely to require fewer mental gymnastics to fully understand, compared to a longer method. I am less likely to need to invest significant amounts of time on each occasion I encounter it.

The Open-Closed Principle says that we should endeavour to design our applications such that new features can be added without the need to change existing code — because that state protects the investment we’ve made in that existing, working code, and removes the risk of breaking it if we were to open it up and change it. Maybe it’s also worth thinking of the OCP as applying equally to methods.

• A couple of end-to-end tests that hit the UI and the database, to prove that we have at least one configuration in which those parts join up. These only need to be run rarely, say on CI and maybe after each deployment.
• An integration test for each adapter, proving that the adapter meets its contract with the domain objects AND that it works correctly with whatever external service it is adapting. This applies to the views-and-controllers pairings too, with the service objects in the middle hexagon stubbed or mocked as appropriate. These will each need to run when the adapters or the external services change, which should be infrequent once initial development of an adapter has settled out.
• Unit tests for each object in the middle hexagon, in which commands issued to other objects are mocked (see @sandimetz‘s testing rules, for which I have no public link). And for every mocked interaction, a contract test proving that the mocked object really would respond as per the mocked interaction (see @jbrains‘s Integrated tests are a scam articles). These will be extremely fast, and will be run every few seconds during the TDD cycle.

I’ve never yet reached this goal, but that’s what I’m striving for when I create tests. It seems perfectly adequate to me, given sufficient discipline around the creation of the contract tests. Have I missed anything? Would it give you confidence in your app?

So I feel it’s the least I can do to raise a little money to help the hospital. And that’s why, in an event that runs spookily parallel to Movember, I’m growing a moustache for money. Your money. Please help the hospital by giving a little at my JustGiving page, or by telling everyone you know and raising my mo’s profile. Thank you.

But while I was writing Refactoring in Ruby with @wwake, and writing Reek at the same time, I began to feel a little uneasy about them. I was never able to put my finger on exactly why that was, or what I was uneasy about, but the feeling never went away. This year I think I have finally understood what I think about code smells, and why I think we can do somewhat better. So before reading on, take a moment to list the things you don’t like about them. Then let’s compare lists. Go ahead, I’ll wait.

Ok, done that? Here are my current reasons for wanting a different tool for describing code quality:

• The names aren’t all that helpful for people unfamiliar with the concepts. If you had never heard of them, what would you make of Feature Envy, Shotgun Surgery, Data Clump etc? Sure, the names are memorable, but that only helps with hindsight, for people who have taken the time and trouble to investigate and learn them.
• Some code smells can overlap. For example, I’m often unsure whether I have seen Feature Envy or Inappropriate Intimacy, Divergent Change or Large Class. There’s a sense in which this doesn’t matter, of course; but it undermines their use as a communication tool.
• Some of the smells can be subjective or contextual, leaving the quality of the code open to interpretation. For example, how large is a Large Class or a Long Parameter List?
• Some of the smells apply only under certain circumstances. For example, a Switch Statement is perfectly fine at the system boundary when we are figuring out the type of an incoming message, but often bad news when it represents a type check on code we own. And it can be acceptable at the system boundary to grab the fields of a Data Class in order to display them, while elsewhere that might be seen as Feature Envy.
• The list of code smells is not canonical; different people have added their own smells to Beck & Fowler’s original list. This situation is even worse when it comes to smells in unit tests; try looking for a canonical list of test smells and you’ll find no consensus whatever. In my opinion, this fact alone completely undermines the idea that code smells form a pattern language for describing code quality.
• There are no clear and obvious code smells covering some dynamic problems, such as the coupling between variables whose values depend on each other, or the problems introduced by mutable objects.

Did you have the same list, or something similar?

So, what can we do about it? I think the answer lies with Connascence. This is an idea introduced by Meilir Page-Jones in two books in the 1990s, and later popularised by @jimweirich in a series of conference talks. I’m not going to cover Connascence in detail here — you can find it all for yourself by reading @jimweirich‘s articles or looking at my summary slides. I just wanted to take a moment to write down my current opinions about code smells. I’ll probably write in more detail about Connascence in the coming weeks, but for now what do you think?

Picture: @rosieemackenzie

The true picture is a little more complicated than that, but the principal ideas are there. The rake task acts as a mediator, allowing me to send messages to (ie. call methods on) my application’s objects.

In general, we want our Adapters to be as thin as possible (and no thinner). That’s because the Adapter code inherently depends on some framework, and that fact will usually make its tests difficult and/or slow. We still need those tests, but we want to have as few paths through the Adapter as possible, and thus fewer tests of the Adapter, so that the total test complexity and test run-time are minimized.

As an example, suppose I have a rake task that validates the posts and comments on my blog:

I want to maximize the unit tested percentage of the code executed during the task, so I move the code out of the rake task and into a new domain object. That new object “is” the task, and usually my rake tasks can then slim down to a single line of code.

Stripping all of the code out of the rake task and moving it into a domain object is analogous to the approach currently being explored by @mattwynne and @tooky in their Rails controller refactorings. In my own code I call these task objects “use case objects”. Currently I keep them in a UseCases namespace within app/models, although I’m open to exploring alternative conventions. One of the nice side-effects of this is that I sometimes discover synergies between the work done by rake tasks and that done by controllers. By pulling Use Case objects out of both kinds of adapter I’m creating a convention for some of the code that turns out to be cleaner, more (quickly) testable, and which names things better.

Please let me know if you try this — or indeed if you’re already doing it.

Today I’ve been exploring ways of improving the unit tests in one of my Rails apps. Let’s pretend it’s a blog app, and I have to add a feature to publish posts. I’ve pulled some code out of a controller to make a UseCase object:

(Here ui is a controller, and makes decisions about routing and rendering in response to the events sent by the use case object.)

I’m really sick and tired of looking at code like this. It seems that most of the classes in any Rails app know that finders return nil when they can’t find something, and so they all have to cope with that case. I want to get rid of that if, or at least hide it away in a single place so that it doesn’t contaminate the rest of the app. I want the test to be a bit simpler and more readable too. And I want the option to have richer finders, maybe coping with fuzzy edge cases and still finding the desired database record. In short, I want to wrap the finder in an Adapter.

So I experimented with a couple of alternatives to the above. First, a variant on Smalltalk’s ifTrue:ifFalse message argument style:

I don’t like this because it’s near impossible to isolate and test the interactions. For example, Blog.should_receive(:lookup_post).with( … what?

Another approach might be something like this:

Indeed Rails itself uses this style in its controllers, and so does the inherited_resources gem. But it suffers from the same testing problems as my previous attempt. So I finally settled on this:

This version uses an explicit listener object, which I can thus instantiate and test independently of the calling context. (In real code I might make it an inner class too.) And I’ve already found a couple more uses for the lookup_post method, removing dependencies and ifs along the way.

I find this approach highly readable and testable, nicely separating different responsibilities and allowing me to give them names. But that doesn’t mean I’ll stop exploring. Have you tried other approaches? How did they work out?

Update

Luke points out that I forgot to include the option I actually use the most, the Self Shunt:

This is the same as the listener option above, but the UseCase object passes itself as the listener, thus saving a class and in my opinion) improving readability a little. Can’t think why I forgot it, cos it’s the best and neatest of them all. Anyroadup, thanks Luke!

You can find out everything you need to know by visiting our page on Eventbrite, where you can find FAQs, links to further information, and the all-important sign-me-up button. Oh, and it’s FREE to attend.

Places are limited, and tickets are going fast now, so make sure you book your place soon!

$ rails server
=> Booting WEBrick
=> Rails 3.0.3 application starting in development on http://0.0.0.0:3000
=> Call with -d to detach

Exiting
/path/to/gems/activerecord-3.0.3/lib/active_record/connection_adapters/mysql_adapter.rb:30:
in `mysql_connection': undefined method `init' for Mysql:Class (NoMethodError)

(followed by a huge stacktrace). Googling didn’t help much, because every time anyone has asked about this on a forum, all of the answers are specific to Macs.

It turns out that the problem is that the mysql gem includes native code, which is built when you install the gem. And my existing mysql gem had been built against mysql libraries that are now, since my upgrade, out of date. So the fix turns out to be maddeningly simple. First, get rid of the gem that doesn’t work:

$ gem uninstall mysql

Next, get hold of the developer API libraries for mysql:

$ sudo apt-get install libmysqlclient16-dev

And finally, download and rebuild the gem again:

$ gem install mysql

Simples. I just hope this quick note saves you the hours of trawling through forums that I had to endure this morning.

Cory stops his analysis when he has the following model:

|Cell| <------- |Status|
           |Alive|    |Dead|

At this point Cory says

“Certainly if we implemented a Cell with a Status object with an AliveStatus class and a DeadStatus class someone, somewhere, should slap us upside the head.”

However, I’d like to carry on from that point just a little further…

I like to teach the use of CRC for exploring design, and so I naturally looked at Cory’s objects from the point of view of their behaviours and responsibilities. Thus it struck me that if we don’t make the split suggested by Cory, we’d have a Cell that knows about its position/neighbours and knows what change to make when the clock ticks. That would violate the Single Responsibility Principle. So the above split into Cell and Status makes sense from the point of view of responsibilities, if we give the Status object the job of deciding how the Cell should change.

With those responsibilities in mind, a little mental CRC session suggests that, when the clock ticks, the Cell would tell its Status object how many neighbours it had, and ask for a new Status object in return.

class Cell
  def change
    @status = @status.update_based_on(@number_of_neighbours)
  end
end

The Status objects become Strategy/Policy objects, and thus don’t seem to need any instance state. All of which means that the Status objects could be Flyweights, with only one extant instance of each type, referenced by every Cell that happens to be in the corresponding state.

I think this approach could well be worth a punt during the upcoming Global Day of Coderetreats…

1. Uses, in which I care only about your public API;
2. Creates, in which I also need to know your class;
3. Inherits/extends, in which I can also see some of your workings and I become part of you.

Don’t do that last one. Ever.

Many of the GoF patterns help move code up this list, from tight towards loose coupling. Except TemplateMethod, which not only encourages inheritance, it also creates a circular dependency by having the superclass only “work” in the presence of extensions. Bad.

Don’t inherit code. Bad.

Here’s (a drastically simplified version of) some code I wrote a while back. (It’s from a Rails app, but the message in this post applies to any technology and any kind of system input.)

class Users::RegistrationsController < Devise::RegistrationsController
  def create
    user = create_new_user_account
    if params[:invitation_token]
      sign_in_as(user)
      redirect_to Invitation.find(params[:invitation_token]).shared_content
    else
      redirect_to new_user_session_path
    end
  end
end

This route handles incoming user registrations, and it has a conditional branch to cater for the case in which the new user was invited, via email, by an existing user. If the new user arrived by clicking a link in an email we do one set of things; whereas we do a different set of things if the user got here via the ‘Register now’ button on the home page. Perfectly reasonable, I thought. Sure, it’s conditional code; but it’s sitting on the system boundary and therefore it’s fine, yes? No.

This conditional check represents duplication. Somewhere else in my application already knew, at some time in the recent past, which route should be taken through this method when it was finally invoked. When the app sent that email invitation, it was in a sense delegating to the recipient the task of supplying user account details. The conditional branch that handles the invitation is a callback from across a technology boundary, and the inviting code already knew to expect it.

The duplication also has a nasty side-effect: high complexity. This method is an order of magnitude harder to understand and to test(-drive) because of the conditional construct. (And you’re reading a version from which I’ve elided all manner of error handling.) Oh, and by forcing two routes into the same method I also made it violate the Single Responsibility Principle.

Happily the duplication is really easy to remove. The code that sends the email invitation knows which path it wants the registration code to take, so I just need to create a new input route for “invitation acceptance” and change the email invitation to link to it. As a result I’ll have two controller methods, each with simpler logic than their predecessor:

class Users::RegistrationsController < Devise::RegistrationsController
  def create
    user = create_new_user_account
    redirect_to new_user_session_path
  end
end

class InvitationAcceptancesController < Devise::RegistrationsController
  def create
    user = create_new_user_account
    sign_in_as(user)
    redirect_to Invitation.find(params[:invitation_token]).shared_content
    end
  end
end

So I have now removed the duplication, and the controller code is simpler as a result. These controller methods are also much easier to test(-drive) than the earlier code. And as a final bonus, I can design the user interface to this route with more knowledge, maybe adding some details of the invitation so that the incoming potential user feels a little more at home. In short, now that each code path has its own method (and class) I can pay it more attention, and it will flourish as a result.

Now, having refactored this code, I take away a few points for the future:

• Where appropriate and possible, model system outputs as delegations into a space that your code can’t reach or control; and model the resulting future input as a response.
• Where a system input is due to the system having earlier delegated a responsibility across its boundary, make sure the return route is differentiated from all others.
• Differentiated incoming routes have code that is easier to read and simpler to test.
• When an input route has a single use, the user experience can be tailored and enriched.
• Not every conditional at the system boundary tests external state.

But now I’ve turned spork off, and here’s why: Spork solves the wrong problem.

We enabled guard/spork because our specs were slow, and during the few months we had spork, those specs became even slower. But we never noticed. Most of the time guard/spork guessed correctly which specs to run, and we became quite good at configuring guard to re-load Rails when critical files changed. So we were reasonably comfortable, and our specs ran quickly most of the time. But our design was getting worse every day.

Slow specs are a sign of too much coupling, and in the case of a Rails app that usually means not enough separation between the domain objects and the adapters. So we’ve stopped running spork, so that we can feel the pain of the 15-second Rails load time, and so that we can feel the pain of all that coupling. And slowly we are making the app’s specs faster. Many spec files now run in under a hundredth of a second, and more will follow as we gradually peel domain code out from the Rails infrastructure. We don’t need to test the Rails components, and we have a very good suite of cucumber features that act as end-to-end integration tests. So our specs should be fast, and only need to tell us that our own objects each do their thing. That’s what we are now working towards, and that’s the pot of gold that spork was hiding from us.

$ rspec spec
#...
Finished in 61.82 seconds
475 examples, 0 failures

61 seconds!  (And on top of that I have to wait another 15 seconds for Rails load; that’s a whole other story, and I hope to come back to that in a future post.) A quick dig into the 61 seconds reveals this:

$ rspec spec/controllers
#...
Finished in 22.82 seconds
114 examples, 0 failures

It happens that almost every controller action in our app needs the user to be logged in, so every controller spec needs that too. We’re using Devise, so every spec has something equivalent to this:

before :each do
  sign_in Factory(:user)
end

This implies hits on the database to create the user object, record the IP address and last sign-in time, etc etc. We can do much better by using a mock_model for the User, and stubbing out Warden‘s authentication completely.

First, in the interests of DRYness, I made some helper methods to do some of the jobs that factory_girl does:

module RandomHelpers
  def random_number() SecureRandom.random_number(10000); end
  def random_id() random_number; end
  def random_name() SecureRandom.hex(20); end
end

Next, a method that creates a test double representing the User who will be logged in:

module ControllerHelpers
  def user_double(attrs = {})
    user_attrs = {
      :first_name => random_name,
      :last_name => random_name,
      :authenticatable_salt => 'x'
    }.merge(attrs)
    mock_model(User, user_attrs)
  end
end

This is the place to fill the User double with default attribute values. Our app requires every User to have a first and last name, so I’ve stubbed these with random strings; they can be overridden by the caller if specific values are needed for any test. In your own app, change this method to set any mandatory attributes your user model needs. Note also the stubbed authenticatable_salt() method, which wasn’t required when mocking earlier versions of Devise.

I also need a method that logs a user in:

module ControllerHelpers
  def stub_sign_in_with(user)
    request.env['warden'] = double(Warden,
                                   :authenticate => user,
                                   :authenticate! => user,
                                   :authenticate? => true)
    sign_in(user)
    return user
  end
end

This replaces Warden with a test double, and Devise is none the wiser.
I also create a method to glue these together for the simplest (and most common) case:

module ControllerHelpers
  def stub_sign_in(attrs = {})
    stub_sign_in_with user_double(attrs)
  end
end

(Note that this returns the User double; you’ll see why that’s useful in a moment.)

Finally, I configure the specs so they have access to these helper methods. To achieve that, spec/spec_helper.rb needs to look something like this:

Dir[Rails.root.join("spec/support/**/*.rb")].each {|f| require f}

RSpec.configure do |config|
  config.include RandomHelpers
  config.include Devise::TestHelpers, :type => :controller
  config.include ControllerHelpers, :type => :controller
end

Putting all this together allows me to write controller specs with:

before do
  stub_sign_in
end

or, if I need access to the user double:

let(:user) { stub_sign_in }

And the payoff?

$ rspec spec/controllers
#...
Finished in 7.36 seconds
114 examples, 0 failures

I saved as whole 15 seconds! There’s still some work to do in my specs, but stubbing out Warden has made a massive difference to every test run that involves this project’s controller microtests.

Note that the above has been tested only with the following gem versions:

• rails 3.0.3
• devise 1.3.4
• warden 1.0.4
• rspec-core 2.6.4
• rspec-mocks 2.6.0
• rspec-rails 2.6.1
• factory_girl 1.3.3

A condensed gist for the above code is here.