200-line functions

I took over a store last month that somebody else has been maintaining since 2012. It’s Magento 1, which by now means a platform plus six years of somebody’s opinions about it. The file that does most of the work is about 1,400 lines of PHP, and the function at the middle of it, the one every page ends up calling, is 211 lines long.

I know that because I counted, which is the sort of thing you do in the first hour when what you’re really doing is having a feeling.

Contempt is not a strategy

The feeling was contempt, obviously. 200 lines. No tests. Four levels of nesting. A variable called $tmp2.

And I’ve done this enough times now to know that the feeling is not only unhelpful, it’s usually wrong on the facts. The people who wrote this were not worse at their jobs than me. They were working on the same code for four years under deadlines I don’t know about, with requirements I haven’t been told, and every strange thing in that file is a fence in a field, and I have no idea what the fence is for.

More practically: I am about to be paid to change this thing. Contempt makes me want to rewrite it, and a rewrite is the single most expensive way to discover what a piece of software actually did.

Every strange thing in that file is a fence in a field, and I have no idea what the fence is for.

So I’ve ended up with an order I read things in, mostly to stop myself forming an opinion too early. It’s not clever. It’s just fixed, which is the point, because the thing it’s protecting me from is my own first hour.

The order of operations

Not the code. The site.

I click around it as a person for ten minutes before I open the editor at all. What does it sell, what are the pages, what happens if I put a bad email in the form. You cannot understand the shape of a codebase without the shape of the thing it does, and I’ve watched myself read three files deep into something that turned out to be an unused feature.

Then the git log, still not the code.

first hour

git log --stat --since="1 year ago" | less   # what changes, and with what
git shortlog -sn                             # who, and how many of them
git log --oneline | wc -l                    # how much history is even here
git log -1 --format=%cd -- path/to/file      # is this file alive or fossilized

Shell

This is the highest-yield hour available and I ignored it for years. The log tells you things the code physically cannot. Which files change together, and therefore which things are secretly one thing. Which file gets touched every two weeks, and is therefore where the actual work of this project happens. Which file hasn’t been edited since 2013, and is therefore either rock solid or dead, and you can find out which in one grep.

It also tells you how many people are in here. 11 contributors over four years, none of whom are still available, is a very different project from two people who are both on email, and it changes what I’m allowed to assume about consistency.

Then the manifest and the build. composer.json, package.json, the gulpfile, whatever config sits at the root. Not to run it, just to read what the project thinks it is. A dependency list is a statement of intent, and where the intent and the code disagree, that gap is usually where the interesting history is.

Then the entry points. How does a URL become a file. Routes, front controller, the .htaccess, whatever it is. This is the only reliable map, and until I’ve got it I’m just reading paragraphs from a book with no page numbers.

Then the templates. Deliberately before the logic, because the output is the part I can verify. I can load a page and see whether I understood it, and being able to check yourself cheaply is worth a lot when everything is unfamiliar.

And then, finally, the 200-line function. With a question.

Read with a question, not to understand

This is the bit that took me longest and it’s the one I’d actually pass on.

“Understanding the codebase” is not a task. It has no edge and no completion condition, and if that’s your goal you will read for three days, feel worse, and have retained none of it. I’ve done exactly that and billed for it.

What works is arriving with something specific. Why does the price show without sales tax on this one page. Where does that email get sent from. What happens between the form submitting and the record existing. Then you read only what’s on that path, and you follow it end to end, and at the end you know one whole thing rather than nine partial things.

Do that four or five times on different questions and you’ve got the codebase, sideways, in a way that actually stuck. It’s slower to feel like progress and much faster to be it.

Don’t touch anything

Refactoring while reading. This was my worst habit and it took a bad week to break it.

The urge is enormous, because tidying feels like progress and reading doesn’t. So you rename $tmp2 while you’re passing, and pull four lines into a helper, and by Thursday you have a diff that touches nine files and does nothing a client can see, with no tests underneath it, and you can no longer tell which of your changes broke the thing that’s now broken.

What I do instead is keep a list. Same markdown file. Every time I want to change something I write the line number and what I’d do, and I change nothing for the first two weeks. About a third of the list turns out to be wrong once I understand why the code is like that, which is a very cheap way to be wrong.

When I do finally split that function, it’s only because I have to change part of it, and I only pull out the part I’m changing. New behavior goes in a new function that the big one calls, so the risky bit is small and isolated and I can look at it on its own. The rest of the 200 lines stays exactly as it is, being ugly and working.

See email from Dave

For the record, the 211 lines were three things stapled together, which is what it always is. About 60 lines of gathering data, about 90 of formatting it for display, and 40 of a very specific pricing rule with a comment above it saying “do not change, see email from Dave 04/14.”

Dave’s email is gone. But that comment is the most valuable thing in the file, because it’s the only part that tells me a human being was surprised here once, and it’s exactly the bit I’d have cheerfully simplified in week one on the grounds that it looked wrong.

It does look wrong. It’s also the business.

Not a methodology

None of this is a methodology, it’s just an order, and the order exists to buy time. Read the log before the code, the templates before the logic, and the opinions last, because the opinion I have in hour one is always the same opinion and it has yet to be correct.

200 lines isn’t a smell. It’s a message from someone who ran out of Thursday, and I’ve sent a few of those myself.