Cascadia Ruby Conf 2012 Therapeutic Refactoring by Katrina Owen

Few things draw my attention
like a looming deadline.

Some part of my brain
watches in morbid fascination

as the deadline approaches

wondering whether to
call emergency services;

or maybe just settle in,

make popcorn,
and watch the show.

I can't help but keep a wary eye on it.

A wary eye that is no longer
paying attention to the code at hand.

I need all the wary eyes I can get.

Without them, forget best practices.

I revert to less successful strategies

like guessing;

and desperately copying code off
of Stack Overflow.

Sometimes I'm happy.

There are moments when
the world melts away.

Your sense of self dissipates.

You become
completely absorbed in what you're doing.

Time appears to slow down
and speed up simultaneously.

Being awesome feels effortless.

I've taken to coming into
the office early in the morning

and committing random acts of refactoring.

Some people are calling this
guilt-driven development.

But really it's not.

Refactoring just makes me happy.

More refactoring is an optimization.

So today I am going to tell you a story.

The arrow needs to point the other way
because this is a refactoring story.

Anyway it has a middle...

Actually it has a beginning,
two middles and an end.

And a moral.

So to be clear, before I get started,

when I say refactoring, I mean

small careful steps that improve the
structure and the readability of the code

without changing its behavior.

Tests are implied.

So.

Once upon a time

there was an application that
performed some incredibly dirty hacks

in order to deliver data straight
into a number of real-world,

old-school, hardcopy publishing systems.

I found this particular specimen
in the dark recesses of that codebase.

It was in a module that
was 300-or-so lines long

most of which was in a single method.

This module talked to code
all over the application.

It dealt in temporary files, FTP;
it shelled out;

used Exif 2 to write
metadata into JPEG files;

it handled encoding.

It had a comment in it that read:

"A kitten dies every time
this code is run."

We had it running on a cron job.

It had no tests

and there was no documentation.

This is essentially
a bucket of business logic.

It jumps through hoops
in order to name files

so that they end up where they need to be.

Now there's a comment

but it's almost as bad as the code.

Also it's wrong.

The input to the method is a target,

which is the God object
in this application.

It's an ActiveRecord model.

That class is 500 lines long.

The big picture here is
that information is being shoved

onto a string in order to
build up the file name.

There is a bit of code that's not actually
shovelling things onto the string.

This draws the eye.

It appears to be a chunk of something.

And it's probably a good
candidate for extraction.

Another striking aspect of the code
is that it's littered

with a bunch of low-level details.

It makes it harder to see what's going on.

But amidst all the clutter

there does seem to be things...

chunks that could probably be named.

So what are we looking at?

We've got a very large method.

It appears to be doing things
at 2 different levels of abstraction.

And nothing at the lower level
of abstraction is being named.

These are not
characteristics of good code.

The thing to do with big ugly code
is to break it apart into small ugly code.

There's a critical difference
between breaking code down,

and breaking code.

The first middle of this story
is gonna be about adding

characterization tests to the method.

And then the second middle is
the actual refactoring.

Where do you even begin
testing something like this?

We don't really know
what the inputs look like.

We certainly don't know
what the output looks like.

We do know that it's in production,

and it appears to be working

because we haven't had any complaints
from the customer about it.

The easiest way to discover inputs
is to just send something—

anything really—into the method

and then see what comes back out.

We need an assertion to work against.

And again, it doesn't matter
what that assertion is.

It is going to be wrong.

The nice thing about being wrong
is it tells you what 'right' looks like.

Running this fails, obviously.

The error message tells us
that we're missing an input.

It also points us to the exact spot
in the code where this is happening.

What we find on line 6—
it comes as no surprise at this point—

the message publish_on
is being sent to target

and it returns a date.

Meanwhile back in our tests,

we know that the stub represents a target.

And we are ready to start
fleshing out those inputs.

The one we need right now is publish_on,

which we know is a date.

And the failure tells us
what our next input is.

Digging around in the code reveals

that xyz_category_prefix is a string.

Next up is kind, which is also a string.

personal is a boolean.

id is an integer.

And title is a string.

And this gives us our first real failure.

Meaning that we've gotten
all the inputs right.

These are the inputs.

The messages that we need
to send to target

in order to build up a file name.

And with the inputs in place,

we're also given the output,

which looks like this.

And now all that practice
copying-and-pasting from Stack Overflow

comes in handy.

Copy that string from the failure into
the assertion and the test should pass.

It doesn't.

The fix for this is to use a regex.

This gives us our first passing test.

We're not quite done yet.

There are a bunch of low-level details
that aren't being exercised.

And there are alternate paths
through the method.

We need to improve the inputs
for these 3 lines of code

and add test cases for the lines
that have conditionals in them.

At every step, failing tests tell us
how to tweak our assertions.

These are trivial to fix; boring to watch.

So I'll show you some highlights.

publish_on gets zero-padded,

and pi doesn't actually
get affected by this

so we'll fall back on e.

kind gsubs out underscores.

We need an underscore
so we can see it being removed.

title appears to strip out everything.

And the existing input
doesn't have very much cruft in it.

So we need some numbers
and some funky characters

and a couple of upper-case letters.

There's something fishy about that regex.

What's with the square brackets?

I'm too lazy to reason about this

so I'm adding a test case,

overriding the stub's input for title,

giving it some square brackets.

And that proves that
brackets get left in place.

This is probably not
the intended behavior.

This test now serves as documentation

until we can clarify this
with the customer.

The first conditional deals in
personalization

and our stub doesn't personalize.

So we need a test for the case
that actually does.

We're informed about yet another input
that's missing.

It's trivial to supply.

The other conditional on that line of code
provides a fallback in case age is nil.

And the test for this stubs out
the same inputs as the previous one.

The final conditional is
a ternary statement

that is trying to determine
where to truncate the title.

Our default input for the title is
neither long nor short.

So by making it longer
we can make sure that it gets truncated.

And then the only case left to test for
is a title that is too short

to get truncated.

At this point we have
protection against regressions.

What have we actually accomplished so far?

We took a piece of
undocumented, untested code.

And with a bit of hand-waving,

we got fake assertions
to give us the inputs.

The inputs gave us the outputs.

And the outputs
gave us the real assertions.

It's almost karmic.

Then we had to reason about the code.

We inspected every line.

Made sure that we had inputs
that would actually exercise those lines

and then we made sure that
every branch of every conditional

was called from a test.

There's no specification here.

There's no design being done.

We're getting a regression test suite.

We are also getting documentation
up to and including the fact

that we probably have a bug.

The biggest win at this point
is that we can start changing things

without breaking into a cold sweat.

So here's what we're gonna do.

We're gonna isolate the method.

And then we're going to
extract a bunch of smaller methods.

I'm gonna borrow a detailed prescription
from Martin Fowler's book Refactoring,

called Replace Method with Method Object.

Traditionally you do this refactoring
when you have big computation

and a bunch of temporary variables

and you don't want to be passing
those temporary variables around

as you extract methods.

Here, we pretty much only have
this one target to worry about.

So first of all,
we need to make a home for this code.

Add an initializer that takes the target.

Add an attribute for it.

And just copy-paste
the whole thing in there.

The method shouldn't be called
on the class.

It needs a better name.

And it no longer takes any arguments.

Back in the old module,

we need to reference the new method
from the old one,

so just delete the whole body
of the method,

reference the new file,

instantiate the class,

pass in the target,
call name on it,

and we're green.

Which means that we have
a license to go to town on this code.

Where to begin?

You could start anywhere.

I always like to delete something.

Let's go ahead and
get rid of that comment.

And now somewhat arbitrarily,

I've chosen to go
from biggest to smallest.

The biggest chunk
that we've seen for extraction

is the truncated title bit.

The actual mechanics of
a method extraction is as follows:

Create an empty method
and name it by what it does

or what it is,

not by how it does it.

Coming up with a good name
is often the hardest thing

you'll ever do in a refactoring.

The next step is to copy the lines of code
from the source method into the new one.

Scan the new method for local variables.

filename here is declared
in the source method,

so we have a choice to make.

We can either do the mutation here,

which means passing the filename in,

or making it an instance variable.

Or we could make this a query method.

Now the whole point of the refactoring is
to separate the 2 levels of abstraction—

building up the filename in one place

and putting together all the little pieces
in another.

So we're gonna just return
the value that we need.

The other temporary variables here
are all local to this method.

We need to assign the result
of our new query method

and delete the temporary variables
that are no longer used

in the source method.

And then we're green again.

Before we move on

I'd like to tidy this up a bit.

There's some unnecessary work
going on in the regex.

We're doing a case-insensitive match,

and then we're downcasing afterwards.

That's kinda backwards.

There's also something in the
match-if brackets that drives me nuts.

The spurious parentheses have got to go.

Another thing that bugs me is
the fact that we have a ternary statement

to decide the range for the match-if.

A match-if is not going
to raise an exception.

If you try to truncate a 4-character
string to 9 characters,

it'll just return 4 characters.

The ternary statement can go.

Since we no longer need truncate_to,

we don't need to worry
about the length of the title.

So that can go too.

And with those 2 lines gone,

there seems very little point
in having a temporary variable,

so we can ditch that as well

effectively leaving us with
a single line of code in truncated_title.

The longest line of code
is now the hexdigest stuff.

Now, hexdigest is a terrible name
in this context.

Copy stuff in.

Assign the result.

And we're green again.

The personalization line
is now the longest.

Perform a quick extraction.

publication_day

and xyz_category_prefix.

Inside this class, xyz is redundant.

The fact that it's a prefix is irrelevant.

So we can drop the xyz prefix

as well as the prefix suffix

and just call the new method category.

And then we've got kind.

This leaves us with
a fairly readable method.

We could totally leave it at this

but there's still
some spurious stuff in here.

publication_day for instance.

publication_day returns a string.

So, why are we interpolating it?

Same goes for category

and kind.

target.id is an integer

but string interpolation
implicitly calls to_s on it.

This next move might seem
a little bit subtle

until you recognize that kids' game
Five Differences.

Which line of code here is different
from all of the other pieces?

The first line was the only one that's
not being shovelled onto the string,

so we can start off with an empty string

and then make the first piece
just like all the others.

Up to a point.

The extension is also different.

If you conceptually separate
this into 2 jobs—

building up the filename,

and then slapping the extension on it

—we're left with 2 distinct sections:

one where everything is smooshed together,

and another where the pieces
are separated by underscores.

If we extract
the smooshed together pieces,

then all the remaining pieces
are separated by an underscore.

We can now shovel them into an array
rather than a string.

This gets rid of a bunch more
of the interpolation syntax,

joining them with an underscore.

Done.

Is it perfect?

Of course not.

Is it better?

Hell yeah!

We went from this to this
in less than 30 steps.

Let me just show you that again.

Before.

After.

Before.

After.

So first we quarantined the method.

Then we extracted stuff.

Extracting stuff basically boils down to

identifying a piece of code
that performs a subtask

and then giving that code a name.

Back when we started out,

we identified 3 code smells:

a large method,

2 different levels of abstraction,

unnamed abstractions.

Abstracting methods addressed
all 3 of those issues.

At that point we still had
some ragged edges though.

So we kinda picked through it

and removed pointless cruft.

This concludes the second middle
of the refactoring story

and I'm gonna end by taking a closer look
at pointless cruft.

In the field of information design,

chartjunk refers to visual elements
in charts and graphs

that add noise.

They don't help you understand the data.

They often get in the way of
comprehension.

To give you an idea of
what we're talking about

this graph gets just about
everything wrong.

This is the exact same data
presented without chartjunk.

Codejunk is the Ruby equivalent
of chartjunk.

This is not about coding practices.

It's not about naming and SRP
and small methods and DRY.

It's about noise.

I've tried to classify codejunk
and I came up with this list.

#10

Does a bear shit in the woods?

Comments shouldn't echo
the implementation.

They shouldn't be wrong.

They shouldn't be imprecise.

And they shouldn't be misspelled.

#9

It is my conviction

that everyone should make their editor
show them trailing whitespace.

If you leave it in,
you have noise in your code.

If you then take it out,
you have noise in your diffs,

which is just wrong on so many levels.

#8 If it's dead, let it rest in peace.

What were you saving it for?

You need to learn to let go.

What is this doing in source control?

#7

More parentheses.

This is vile.

When I see this,
I secretly wonder if you

wash your hands after you
go to the bathroom.

#6 Intelligent defaults.

Yes. 4 characters saved are 4 characters
that you can spend another day.

Or something.

Let's talk about dependencies.

In particular, dependencies that
you're not actually depending on.

This may or may not impact performance.

It probably will impact the performance
of your tests.

A whole different story.

Mostly you just added noise to the code.

#4 This is familiar territory.

Recognize this?

Not to beat a dead horse,

but ew.

#3 Don't do work that the computer
is already doing for you.

More string interpolation.

I could've technicallly folded this into
the previous one

but I'm trying to get to 10.

This is the same thing.

The call to map is superfluous here.

The match-if brackets
don't need your help.

We've seen that one.

In this example the computer isn't
doing the work for you but should be.

#2 Test suites.

They should be curated, maintained, fed,
watered, coddled and minimized.

The only exception I can think of is if
documenting behavior outweighs the cost.

Choose wisely.

And finally, the compounding sin.

You know the old saying,
"1 + 1 = 3 for very large values of 1"?

This is revolting.

In fact, it bears a horrifying resemblance
to the code that

we started out with today.

When people say that
software is grown, not built,

I'm pretty sure that this
is what they're talking about.

It's kinda like fungus.

Earlier I said about the refactoring
that after we extracted methods

we kinda polished up the code a little.

I'm gonna restate that
in light of the previous section.

First, we extracted methods.

Then we eliminated codejunk.

That was the ending of
the refactoring story.

Before I get to the moral,
I'd like to mention

the whole thing lives on Github.

It was committed step-by-step,

so if you wanna look
at any of the mechanics

or run the tests or anything,

just dig through the commit history.
It's all there.

I have a vague recollection of the night
when that code was written.

Let me rephrase that:

I vaguely recall writing it.

Yeah.

When I panic, I write godawful code.

Other people do yoga, or meditate,
or go white water rafting.

I refactor.

It soothes me.

Science—if you believe that sorta thing—

actually explains why.

The key player in the game
is working memory.

Working memory is the thing
that allows you to do mental arithmetic.

You store temporary results off in memory
as you work through

other parts of the problem.

The main differentiator in performance
of tasks that rely on working memory,

is the type of strategy
you're able to apply to a problem.

If you have better working memory

you can apply more complex,
more successful strategies.

Here's the deal:

when you worry in ways
that involve mental chatter,

you use up available slots
in your working memory.

You no longer have the swap space
necessary to use complex strategies

and you revert to less successful ones
like copying stuff off of Stack Overflow.

This is where refactoring comes in.

Refactoring makes you smarter.

Refactoring basically gives you
an exobrain.

You offload a bunch of those
little details—that

under normal circumstances
go into working memory

—into your tests.

Once you start refactoring,
you start reclaiming your brain.

It actively counteracts panic.

One of the really unexpected things
that happened when I started refactoring

simply for the act
of refactoring itself was

that I started optimizing
for happiness.

I would specifically put something
into its own class

just so that I could load
just that class and nothing else.

The feedback loop you get
when you have subsecond test suites

for the thing that you're working on
is unbelievable.

It's a huge enabler for flow.

Flow is nice.

Flow is more than nice.

It's like a bliss cocktail.

Selfishly isolating code
to improve my own happiness

turned out to be a pretty good deal
for the code as well.

Suddenly I was avoiding dependencies.

I was constantly asking myself

"Can this be a separate class?"

"How about a gem?"

"What is the unrelated subproblem here?"

It made a dramatic difference.

In summary, refactoring makes you smarter
by giving you an exobrain.

It is preventative and curative
with respect to panic.

If you optimize for developer happiness
by making your tests really fast,

you get loosely coupled code that adheres
to the Single Responsibility Principle.

Thank you.