How A Pull Request Rocked My World

This is a story about Pull Request #2 and how it made me rethink my code. This isn't really a story about the code itself; it's about how even a small refactoring can multiply value and productivity. But hey, let's not get ahead of ourselves.

Long ago, there was some code in an iOS library called Formotion that looked like this:

if row.submit_button?
    make_submit_cell(row, cell)
elsif row.switchable?
    make_switch_cell(row, cell)
elsif row.checkable?
    make_check_cell(row, cell)
elsif row.editable?
    make_text_field(row, cell)
end

Basically, it configures this cell object based on properties of the row. That serpentine if/elsif chain did the trick and didn't seem alarmingly unreasonable at the time, so the code shipped with the first version of Formotion. It all worked out, the project got some watchers and star-ers, and folks generally had great feedback. All was well.

But then this little pull request appears a few days later.

I get an email about it, and am immediately excited because hey, someone liked my little project enough to contribute! So I command-click the link open in a new tab and have a look.

The request is labeled Refactoring & Multiline Text Type. So usually when someone comes up to you and says like, "Hey I did some refactoring, check it out," the implicit understanding is that the original thing just wasn't so hot. And about nine times out of ten you agree and know that it's true, but it is a little awkward when someone beats you to fixing it.

Anyway, I inhale whatever pride I have about my work and click the "Files Changed" tab to investigate. I take a gander, and basically what the request does is replace that big fat if/elsif block with this one line:

row.object.build_cell(cell)

Whoa, what is this new row.object? Well, elsewhere in the refactor, this line appeared:

@object = Formotion::RowType.for(type).new(self)

What's all this about? Turns out that the refactor took the various row configurations (switch, submit, check) and created new subclasses of Formotion::RowType for each: SwitchRow, SubmitRow, CheckRow, etc. In those objects, the build_cell method now dictates how the cell is configured, taking the place of the old make_submit_cell methods.

And that RowType.for() method? It actually does a little metaprogramming to grab the appropriate subclass:

# type == :switch or :submit etc
string = "#{type.to_s.downcase}_row".camelize
# string == 'SwitchRow', 'SubmitRow' etc
return Formotion::RowType.const_get(string)

So instead of some if tree and hard-coded methods, everything is dynamic and decided at run-time. You can reuse code with inheritance. Absolutely crazy. Brilliant.

All of this blew me away. Like this "@mordaroso" fellow strolled on up, cocked his knee skyward, and jettisoned his leg into the rickety wooden door that previously sheltered my mind.

So there I am, hunched over my laptop, eyebrows furrowed to the point where my forehead like folds in over itself, just stupefied by this code. And the ramifications, like how it allows for extensibility and plugins and testing and a generally more robust codebase, are just gushing into the void Fabio Kuhn exposed in said cranium.

With just that one refactor, the value of my little library had grown tremendously! If you wanted to make a plugin, there was no messy monkey-patching: simply create your new RowType subclass, like MyWidgetRow, and you're done. It just works.

Not a day goes by where I'm shin-deep in a project and don't think about what that pull request revealed: reorganizing your code (in the right way) can create cascading growth in productivity and value. In Formotion's case, that pattern of dynamic-class-lookup is now used in other parts of the codebase and allows for a project-wide plugin architecture. Big win all around. This sort of stuff really is worth being diligent about.

So thanks, Fabio, for blowing my mind.

Edit 2/3/2013 So a lot of folks drew attention to "You can reuse code with inheritance. Absolutely crazy. Brilliant." and took as equivalent to me saying "It's absolutely crazy and brilliant you can reuse code with inheritance." I get that both in- and especially out-of-context that's how it comes off, but it's not what I meant. It was sloppy and I should've been more careful in writing.

I meant it to come off more like "...everything is dynamic and decided at run-time, plus you get a great plugin architecture if you do some polymorphic tricks with these RowType objects. And all of these benefits came from just one simple refactoring. The power of small refactors is absolutely crazy. Brilliant." See my comment for more context.