Make the Change Easy, Then Make the Easy Change

Confreaks · Aloha Ruby Conf 2012 · Ben Orenstein (Thoughtbot) · Video ID DC-pQPq0acs · 44:19

A sales report that filters orders by placed_at, takes a start date and an end date, and returns a total is already a B, maybe a B-minus. A year earlier, Ben Orenstein would have committed it. The interesting claim is not that the report is wrong. It is that “good enough to ship and never touch” and “good enough to extend” are different grades, and most of the distance between them is a handful of named moves you can make while the tests stay green.

Orenstein, then at Thoughtbot in Boston, gave the talk twice in one week — Thursday at Magic Ruby in Disney World, then immediately at Aloha Ruby in Hawaii — and framed it as pairing, not lecture. Interrupt. Disagree. The room did: an audience member named the feature-envy smell, another flagged missing edge-condition tests, and a developer named Cory argued about inject. Earlier runs of the same talk had already left fingerprints. José Valim, possibly at Scottish RubyConf, had shown him cover?. Jim Weirich had found a bug in the fold.

A temp is a method that has not been born yet

The first move is almost too small to defend in isolation. The report computes orders_in_range, stuffs it in a local, then sums it. Orenstein extracts the temp into a private query of the same name: extract temp to query.

Two one-line methods replace one two-line method. He will not claim that split is always an improvement, but he now treats methods longer than a line as a code smell — something that may indicate a problem, not proof that one exists. The practical wins stack. The next stakeholder request after “total sales within date range” is usually “average sales within date range.” If the filter lives inside the total, it will be copied. If it is a query, it will be reused. And because programmers read code from the top, a private method is a hint: this is an implementation detail. Skip it unless you need it. The public method is what the report is about.

Stop asking Order about itself

The new query still interrogates order.placed_at and decides, on Order’s behalf, whether the date falls in range. That violates tell, don’t ask: send a message and let the object do the work, rather than pulling internals out and fiddling with them. Not a law. A maxim that tends to produce better code.

He writes the code he wishes existed — placed_between?(start_date, end_date) — lets the spec blow up, then puts the comparison on Order. Interior details stop leaking into the report. The surrounding code talks to Order in messages, which is the point of the object in the first place.

If dropping one argument makes the other nonsense, you have a clump

Now start date and end date travel together through initialize, through the query, through placed_between?. That pair is a data clump. Litmus: take one away. An orders report with only an end date does not mean anything. The reliance is implicit. A DateRange makes it explicit.

He starts in the spec, references a constant that does not exist, then a struct that holds the two dates, then threads the new object through until the suite is green. Bob Martin’s complaint, as Orenstein relays it: most intermediate object-oriented programmers are too reluctant to extract classes. DateRange is almost embarrassingly simple. It has, at first, no behavior. It still earns its keep because a name has been created, and because the report’s argument list just went from three to two.

That last point is about parameter coupling. If notify_user_of_failure passes a failure into print_to_console, and the printer calls two_sentences on that parameter, the two methods are coupled through the argument. Pass nil or an integer and it explodes. Zero-argument methods beat one-argument methods; one beats two; two beat three. Each missing parameter is coupling you no longer have. Low coupling is what makes change cheap — and change, Orenstein says, quoting a Dave Thomas keynote, is the only thing worth worrying about when you look at code.

Behavior wants to sit on the data it uses

Once DateRange exists, “is this date between two bounds?” is a strange responsibility for Order. It is a natural one for DateRange. Feature envy is the smell: one class overly concerned with another’s internals. Move the question. Order asks DateRange whether placed_at is included; DateRange answers.

He first writes the inclusion with a range include?, then remembers a correction José Valim gave him after an earlier delivery, maybe Scottish RubyConf. In Ruby, include? can instantiate every object in the range. cover? checks the endpoints. On a three-hundred-year date range the difference is not academic. An audience member notes the tests still do not cover edge conditions. Orenstein agrees on the spot. The refactor is not finished because the suite is green; it is finished when the new object is actually trusted.

The public method should now read the way he would say it to a human: total sales within the date range are the total sales of the orders within range. Map-plus-inject is not how anyone describes the work. He is extra aggressive about that standard on the public API, slightly less so on private helpers, because the next reader mostly cares about the public one.

Cory, from the room, suggests dropping the 0 seed on inject. Only partly right. With no orders in range, a fold without an identity returns nil instead of zero. Jim Weirich had already found that bug in an earlier run of the talk. Orenstein’s commit message, he says, was that Jim Weirich found a bug in his talk.

Nil is a terrible collaborator

The second example is a job site. Every site has a location. Not every site has a contact. That optionality is implicit until you trip over it: contact_name and contact_phone check for presence and supply defaults; emailing the contact guards the call. The methods no longer say what they mean. They say “handle nil, then maybe do the thing.”

Co-opting the nil singleton as “no contact” is tell-don’t-ask again. The client asks whether contact evaluates truthily, then decides. The way out is the null object pattern: an explicit NullContact that answers name with “No Name,” answers phone, and no-ops deliver_personalized_email. If no contact was passed in, assign NullContact.new.

Three conditionals disappear. About twelve lines go with them. Client code always tells some kind of contact to give its name and never cares which kind. The refactoring’s catalog name is replace conditional with polymorphism: send the same message to different types and let them answer differently.

The cost is real, and he says so. You now keep two APIs in sync. Add a method to Contact, add it to NullContact. He still thinks it is generally worth it, because hairy nil conditionals are everywhere — including the Rails classic if current_user. Have current_user return a User or a NullUser, not nil.

Where the class lives depends on how far it spreads. A null contact used across the app becomes app/models/null_contact.rb with its own unit tests. A tiny type used by one class can stay nested and private. He does not test private methods. Ever. Promote it, then test it.

Abstractions are fractal, and gem names are a smell

Most programmers already know to depend on Active Record instead of handwritten SQL, and on Net::HTTP instead of shoving bytes into a socket. What they miss is that you can keep climbing. Abstractions are fractal. The rule of thumb he takes from Growing Object-Oriented Software, Guided by Tests: the whole should be simpler than the sum of its parts. If a cluster of classes is awkward to use, wrap it and give callers a smaller API.

His billing example has User creating a Braintree customer, charging a subscription, finding a Braintree ID; Refund looking up a transaction ID and issuing a refund. The Braintree gem is already an abstraction over an HTTP API. It is not high enough. Changing payment gems should not require opening User. That is shotgun surgery: one idea, thirty files, shrapnel everywhere.

He introduces PaymentGateway, hangs the Braintree constant there, and lets User and Refund know only charge_for_subscription, create_customer, refund. One file changes when the processor changes. Tests stub those methods — methods he owns — instead of stubbing Braintree. Stub other people’s APIs and a version bump leaves tests green while production stops charging people. If a test makes you feel dirty, listen to it. Change the design until the test is straightforward. That feeling is a positive code smell.

Do not refactor because it is Monday

The best time to refactor is when you already need to change the code. Orenstein is a consultant; at the end of the week he has to point at value. He does not wake up and decide User is ugly. Kent Beck’s tweet is the timing rule:

If you need to make a change, refactor the code so that the change is easy (note: this part might be hard), then make the easy change.

What to point that energy at: god objects, high-churn files, and bug magnets.

Rails apps, he says, almost always have two god objects: User, and whatever the app is about. Todo list? Todo. Ecommerce? Order. His anonymized wc -l of app/models shows User, then Order, then Merchant — an app with arguably three. A 600-line class does not have one responsibility. First rule of classes: they should be very small. Second: even smaller than that. When he has to touch User, he is extremely aggressive about leaving it smaller than he found it. Be reluctant to add lines to a god object.

Churn is the other tell. If you keep coming back to a file, you do not understand it yet. A gem named churn will rank files by how often they change in git (or, he jokes, SVN). Give those a hefty dose of refactoring so the next change is cheap.

Bugs love company. A bug on line 10 often means a bug on line 11, because the code was too complicated to see. After you find a bug, simplify the neighborhood so the next one has fewer places to hide.

He closes with books, not slides. One he calls the best intermediate-to-advanced-beginner programming book and does not name aloud. The next is “the Bible”: know the smell names, know the techniques — the description matches Fowler’s Refactoring. He then points at Bob Martin and Martin Fowler as the best software authors he knows. The third, the least famous of the three, he does name: Growing Object-Oriented Software, Guided by Tests, which made implicit OO and TDD ideas explicit and gave him rules of thumb he had not had.

Nothing he taught always works. Null objects cost API sync. Extra classes cost navigation. One-line methods are a detector, not a quota. The test that survives all of that is still Dave Thomas’s: is it easy to change? If not, name the implicit thing, move the behavior onto it, and make the next request a one-line change to a public method that already reads like speech.