How to Refactor a Long, Messy, Badly Tested Controller

At some point in your Rails career, you’ll run into a controller that’ll make you want to give up programming forever. It may contain every line of code for an entire feature. It might have 15 before_filters that all communicate through instance variables and have to be called in a specific order or things blow up. And, inevitably, its tests will look like this:

test "index" do
  get :index
  assert_response :success
end

Awesome. 100% test coverage, right?

As great as it would be to just close your eyes and pretend it doesn’t exist, someday you’ll have to fix a bug in one of these controllers. And, being a good software developer, you want to leave the code better than you found it.

But how can you refactor, especially without good tests to rely on?

Get it tested (somehow)

You need good tests to feel safe while refactoring, but you can’t write good tests against this code until you refactor. So what can you do?

Well, no matter how badly written your controller is, you can still write integration tests that send some data to the controller and expect some response out of it. For now, you should write tests that make sure that the controller’s existing behavior doesn’t change as you refactor.

These tests won’t be as focused as unit tests. But these high-level tests can make you can feel comfortable that the refactoring you’re about to do won’t break everything. And the process of writing them will help you understand your code better, which will help you decide how to refactor it.

Break your dependencies

What makes bad controller code bad? Most of the time, it’s the implicit dependencies between before_filters, helper methods, or different parts of 200-line functions. It could also be a case of refactoring too early. To make the code better, you need to break these dependencies.

For Rails controllers, there’s an easy way to break dependencies. Just copy the code from your before_filters, helper methods, superclasses, and anywhere else controller-ish code could be hiding. Then, replace the calls to those methods with the code you copied.

You’re temporarily un-DRYing your code, so you can refactor it in a more understandable way later. It’s ugly, but now all of your code is out in the open. You should be able to see which pieces interact and how everything flows.

(During this process, you should be running your tests after every change to make sure your inlining isn’t breaking anything.)

Refactor toward testable code

Now, you’re ready to re-refactor your code. You’ll probably stick to the basic extract method, extract object, pull up method-type refactorings. Try refactoring your code a few different ways and see what feels best.

During the first few passes, you can rely on your high-level integration tests as a safety net. But soon, you’ll want something better.

As you refactor, look for opportunities to make your code more testable. This will usually mean creating places to inject test doubles, reducing dependencies between your objects, and making your controller rely on objects that can be easily created and unit tested. By moving code into testable objects, your controllers will get smaller, easier to understand, and easier to test themselves.

Can I get it in the form of a numbered list?

Once again, these are the steps to take to break down a large controller:

1. Get high-level integration tests running against the controller.
2. Run the tests, make sure they pass.
3. Inline before_filters, superclass methods, and other abstractions that hide code.
4. Run the tests, make sure they still pass.
5. Perform a refactoring (extract method, extract service object, replace instance variable with local, etc.). See if the code feels better.
6. Run the tests, make sure they still pass.
7. If you just extracted code, write some unit tests against the object or method you extracted.
8. Run the tests, make sure they still pass.
9. If the controller still needs work, go back to step 5.

What’s next?

If you want to learn more, Working Effectively with Legacy Code is the bible of taking unmaintainable code with no tests and turning it into something you can work with. I highly recommend it, if you find yourself facing huge monolithic controllers often (and if refactoring is as much fun to you as it is to me).

So now, let’s hear your horror stories! What has the worst controller code you’ve ever worked on looked like?