Noel Rappin Writes Here

This is not exactly what the code in question looked like

I had a unique career experience this year. After watching this really quite fantastic RailsConf talk from Katrina Owen, I asked if she’d be interested in doing a full-day refactoring workshop for the Table XI developer team.

Katrina agreed, and we decided that she would put together a workshop based on refactoring some of our actual client code. I had just the project in mind, and had her look at some specific examples.

We settled on one particular feature. Without going into detail, there was a user-facing and administrator-facing version of the same task. The users could do a couple of things the admins wouldn’t need to, and the admins had some flexibility not given to the users. The implementation was an abstract parent class, a concrete class for the user-facing and admin-facing versions, and a few cases where chunks of functionality were broken off into small service classes. Overall, about six files, plus associated tests. The test suite for this feature is complete, but slow.

A lot of this code was written by me, intermittently over the course of three years, in the face of a tight budget, tight deadlines, and complex and changing business logic. So the code isn’t exactly the best thing I’ve ever done. Which is, of course, why it was a great choice for a refactoring workshop.

Katrina’s strategy for this kind of big refactor has a couple of goals. She explained these goals to me in between questions like, “what is this code supposed to do?”, “why are these two use cases different?”, “why did you do that?”, and “no, really, why did you do that?”.

Parenthetically, It was only after Katrina started looking at the code that it occurred to me to be nervous about it. I spend a fair amount of time telling other people about how to test well and how to code well. I think I’m good at that, but it’s fair to say that most people who read what I write take it on faith that I’m a good coder. To have somebody external explicitly looking at a piece of code that I knew was not my best, and asking her to do so with a very critical eye, since we were were looking for refactoring opportunities, was more than a little nerve-wracking.

In the end, Katrina was nothing but gracious about the code, and it felt like the best kind of code review. The kind where somebody skilled points out where you might have cut a corner for time, asks you to defend your assumptions, and shows you cleaner ways of moving forward. The kind where you are excited to have new techniques to try in the future.

One of Katrina’s big concerns when dealing with a large-scale refactoring is safety. Especially on a refactoring that might take days or weeks, you want to be able to confirm that behavior hasn’t changed. Just as importantly, you want the code to always be in a deployable state so that if you get pulled away from the refactor for other work, the rest of the project can continue and you can come back to the refactor when you have more time.

Proceeding safely means both having tests and taking very small, incremental steps. How incremental? Katrina’s rule of thumb is if she has two test failures in a row, she rolls back to the last passing test state and starts over. One advantage of working in small steps, is that a lot of the small steps are relatively mechanical and short, meaning that you can focus your creativity on larger design and code quality concerns.

Having given Katrina the client code, she spent time trying to refactor it herself so that we could present steps for people to follow in the workshop. Once her initial shock had worn off, and she felt she understood the code, her diagnosis was that we had structured the code to the wrong set of abstractions. Part of this diagnosis was based on the fact that there were still conditionals throughout the concrete classes, and even the service objects, which should have been more standalone, also had a fair number of repeated conditionals.

After some really specific cleanup to align the various public-facing API’s of the classes, Katrina unwound the abstractions by inlining them. Literally taking each service class and turning it into one big method, then putting that method back in the base class, then unwinding the inheritance by explicitly copying the inherited code back into the base classes.

At this point, we still had working code, but there was a lot of duplication and a couple of long and unwieldy methods. But she had done something kind of magical to the code: taken out all of our assumptions about how it should be structured. Now we could clearly see patterns that were harder to see when the functionally was poorly split up. And we could begin to look for the signs of better abstractions: attributes that are used together, duplicated conditionals, similar logic with small differences.

We didn’t actually finish refactoring the code, it was too much for a one-day workshop. But the tools and techniques we used: small steps, ensuring safety, inlining code, focusing on differences, are all things that we’ll be able to use in future projects.

My advice, then, is that if you get the chance to listen to Katrina talk about refactoring for a day, you should do it. And you should seek out opportunities to let people you trust look at your code, even the code that you aren’t proud of, it’s a great way to improve.

If you want more specific information on how Katrina approaches code design and refactoring, you should buy the book 99 Bottles of OOP by Sandi Metz and Katrina Owen.