Tips for Working with Legacy Code

Get Comfortable with Git (or JJ)

Version control is a legacy code superpower, but it is one that can take some getting used to!

One tactic is making commits where the only message is “WIP” or “Work in Progress”. These give you checkpoints that you can backtrack to if you find your attempt to fix things has made everything go wrong. You can jump backwards through time with git checkout HEAD~N, where N is the number of commits you want to undo. Have a look around and decide if that’s the right place, then create a new branch with git switch -c new-branch-name and carry on from that point. Later on you can clean up unneeded branches with git branch -d old-branch-name-1 old-branch-name-2.

The big downside of this approach is that your commits aren’t so useful any more. Rather than being whole features with useful descriptions about what’s happening, they become a long list of “WIP” “WIP” “WIP” “WIP”, and your commit history starts sounding like a frog with a lisp. In Git, we can resolve this by squashing the commits together into a single new commit that contains all the changes, ideally with a fresh new commit message that explains everything that’s happened. There are a few ways to do this — the classic is the interactive rebase, which can take a bit of getting used to, but is very powerful. However, if you’re happy squashing an entire pull request into a single commit, most git forges like GitHub or GitLab allow you to do automatically squash just by pressing a button.

The benefit of doing all this is the chance to experiment more with different ideas. If it’s clear that your attempt to fix everything is just making everything worse, then you can back out and start again. And with the approach above of using git checkout and git switch -c, you always have your old branches lying around in case you decide that your initial attempt wasn’t so bad after all.

Creating checkpoints isn’t the only benefit of version control. When working with a codebase I don’t understand, I often reach for git blame. This is a terrible name for a really useful tool: git blame <filename> shows you when each line of a file was last changed, by whom, and in which commit. That last part is the most important, because it means you can time-travel through the codebase, seeing how a particular file changed over time.

For example if I clone the Git codebase, I might do git blame blame.c, to find out how the blame command itself has changed over time. The output looks something like this:

(snip)
a470beea39b (Nguyễn Thái Ngọc Duy     2018-09-21 17:57:21 +0200  107) static void verify_working_tree_path(struct repository *r,
ecbbc0a53b0 (Nguyễn Thái Ngọc Duy     2018-08-13 18:14:41 +0200  108)                                struct commit *work_tree, const char *path)
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  109) {
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  110)   struct commit_list *parents;
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  111)   int pos;
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  112)
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  113)   for (parents = work_tree->parents; parents; parents = parents->next) {
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  114)           const struct object_id *commit_oid = &parents->item->object.oid;
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  115)           struct object_id blob_oid;
5ec1e728237 (Elijah Newren            2019-04-05 08:00:12 -0700  116)           unsigned short mode;
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  117)
50ddb089ff6 (Nguyễn Thái Ngọc Duy     2019-06-27 16:28:49 +0700  118)           if (!get_tree_entry(r, commit_oid, path, &blob_oid, &mode) &&
a470beea39b (Nguyễn Thái Ngọc Duy     2018-09-21 17:57:21 +0200  119)               oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB)
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  120)                   return;
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  121)   }
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  122)
a470beea39b (Nguyễn Thái Ngọc Duy     2018-09-21 17:57:21 +0200  123)   pos = index_name_pos(r->index, path, strlen(path));
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  124)   if (pos >= 0)
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  125)           ; /* path is in the index */
a470beea39b (Nguyễn Thái Ngọc Duy     2018-09-21 17:57:21 +0200  126)   else if (-1 - pos < r->index->cache_nr &&
a470beea39b (Nguyễn Thái Ngọc Duy     2018-09-21 17:57:21 +0200  127)            !strcmp(r->index->cache[-1 - pos]->name, path))
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  128)           ; /* path is in the index, unmerged */
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  129)   else
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  130)           die("no such path '%s' in HEAD", path);
072bf4321fb (Jeff Smith               2017-05-24 00:15:34 -0500  131) }
(snip)

Now, if I’m confused by one of these lines of code, say why this function uses unsigned short mode on line 116, I can do git show 5ec1e728237 and see the commit where that change was made. This shows me the message (which, if I’m lucky, might explain more of the context), and the diff of what else was changed in that commit. I can also see who made the commit, which can be useful if they’re still around and can answer my questions!

Most IDEs also provide this functionality, either built-in or as an extension. In VSCode, I use “Better Git Line Blame”, which shows a little bit of text at the line where my cursor is telling me who made the change, when that happened, and what the commit message was. Hovering over this text also allows me to jump directly to the diff of the commit.

One caveat is that Git doesn’t always make this kind of jumping around easy, which can make it more intimating to get started with. If you’re in this situation, I highly recommend JJ/Jujutsu. This is an alternative version control tool, but completely compatible with Git so that you can use it with your usual project repositories alongside colleagues who are using Git. The big benefit for legacy code is that it makes it very easy to create new, temporary commits all over the place, and jump between them as needed, as well as undo changes more easily (it’s literally just jj undo). If you’re interested in trying this out, Steve Klabnik has a very useful JJ tutorial.

Create a “Tech Debt” tracker (And Use It!)

When working with legacy code, you’ve got to remember that you won’t be able to clean up everything at once, and you won’t ever be able to clean up many things. But you still need to know where the problems lie, even if you can’t fix them right now. A technical debt backlog is great for this, although it needs to be used well, otherwise it can become a graveyard of technical despair.

The most important thing here is documentation. When adding something to the backlog, make clear:

• What the exact problem is (performance issues, a confusing API, code that is difficult to update).
• Which bits of code are affected (useful for keeping track of things later).
• What you’ve already tried to do to fix the problem.
• At least one idea of a suggested solution.

The goal — like with all good tickets — is that anyone on the dev team who’s familiar with that bit of code can pick up the ticket and start working on it.

Once you’ve got the ticket there, you also need to do something about it. As developers, I think it’s often healthy to include space for fixing technical debt in our estimates. For example, if I’ve got a new feature to implement in a certain region of the codebase, and I know there’s a tech debt item in the same region, I might suggest combining the two and estimating the work for doing both at once. This requires buy-in from everyone involved, though, which may not always be possible.

Another approach is to dedicate a certain amount of time each week/sprint/etc to working on tech debt tickets. Again, this requires everyone to accept that the team will be moving more slowly in the short term, for the benefit of improving speed in the long term.

With a tech debt tracker, one of the difficulties is prioritisation. With adding features or fixing bugs, prioritisation is generally a business decision, but with tech debt, the decision is firmly a technical one. A good technique for seeing how much a particular ticket is affecting developers (and therefore how quickly it should be fixed) is to add comments or “+1”s to that ticket whenever you or another developer runs into it again. For physical trackers with post-it notes and pens, I’ve seen this done with coloured dot stickers.

Write Tests (Even If They Assert the Wrong Thing)

I don’t want to beat around the bush here: writing tests for legacy code is hard. Writing good tests is mainly a question of writing clean, easy-to-test code, and the whole point of legacy code is that it is neither clean, nor easy to test!

But on the other hand, working with legacy code involves a lot of refactoring, and tests are a lifesaver for getting refactoring right! So adding tests can be a worthwhile, if arduous endeavour.

The core problem with legacy code is typically that the abstractions in the codebase aren’t quite right, and that to fix this, developers have had to spaghetti link pieces of code together that were never meant to be connected, or thread some bit of code through complex systems so that it’s accessible in another function that needs it.

This means that to write useful tests without running into the dangers of mocking, you often need to write tests at higher levels of abstraction than you would normally. For example, in a React or Vue app, you might need to test a whole component or even page rather than just testing a single hook.

One of the benefits of this style of testing, though, is that it can show the places where a particular bit of logic needs to be abstracted away. If you’re writing tests for a whole component, but there are some specific edge-cases that are really hard to test at that level, then that might be a sign that you should start refactoring those edge cases into their own component/hook/class/etc.

I learned another trick here from a blog post from Matklad: It’s okay to write tests that check for behaviour you don’t want. If you’re writing a test that asserts the correct behaviour, and the code currently does something different and incorrect, consider changing the test so that it now asserts the incorrect behaviour instead. Now, when another developer stumbles across this bit of code, they don’t have to make the same discovery that you did, because there’s already a test and a comment explaining the unusual behaviour. And of course, make sure you add a ticket about this in the bug tracker, and link it in that comment. When you get round to fixing the code, you can update the test to assert the correct behaviour.

Take Time to Understand

One of the biggest dangers when reading any kind of code — even the best stuff — is assumptions. You look at some code, pattern match on the sort of code you expect to see, and forget to read it properly and find out that it’s actually doing the complete opposite of what you thought. A classic example is reading code that updates a bunch of x and y values, and not noticing that some of the code uses y instead of x and vice versa.

When it comes to legacy code, you’re dealing with a lack of familiarity with the codebase, a variety of different styles of code, and the fact that the obvious bugs have already been ironed out, leaving only the really gnarly ones to deal with. This means it’s usually really important to take time to understand what’s going on at every level.

As discussed in the last section, tests can be really useful. Tests give you a chance to write down what you think will happen and see if that’s correct. They also let you document all the places where you were not correct. However, as mentioned, it’s often hard to write useful tests for legacy code, so while this is great when it works, it might not always be possible.

Using a debugger is another useful option. Stepping in and out of functions can be a really useful way of forcing yourself to follow the exact flow of control, looking out for unexpected returns or errors that have surprising effects. If hooking up the code to a real debugger won’t work, the print statement works as a poor man’s debugger.

You should also make notes about everything you’re doing. I’ve got to confess, this is something I’ve never been great at — I did two years of a physics degree where I tried desperately to keep a tidy logbook, before giving up and switching to programming, only to learn that good programmers also keep a logbook! Making notes on paper is great because you can write, draw, and add diagrams, all in the same space, although I tend to find these sorts of notes work best as something temporary. More permanent thoughts could go on a project wiki, or even in a ticket on the tech debt tracker. This makes it possible for the whole team to access the information, but just as important is the chance for you in six months to come back to a ticket and still have a chance of figuring out what you’d done the last time you’d looked at it.

Finally, if things just aren’t making sense, taking some time away from the code can be really useful. Go and do something else (or take a lunch break, or go home), and come back with different eyes to look at the code. This is stupidly effective, and there’s probably some biological reason why it works so well, although I’ve got no idea what that might be.

Timebox Everything

The last piece of advice here is mainly for myself. I really like the feeling of refactoring. Taking something that used to be chaotic and messy and moulding it into something clean and readable is simply amazing. When I can’t sleep at night and I need to go to my happy place, I picture a PR with file after file of deleted code. (That last part is a lie, but it’s more true than I’m comfortable admitting.)

Unfortunately for me, now may not always be the time and the place for a big, in-depth refactoring. After all, the true goal of code is that it does what it’s meant to — that it looks pretty while doing it is mainly just an advantage for those of us working on it. It doesn’t make sense to spend a week refactoring some chunk of code if that code was mostly fine in the first place.

To counter my natural urges here, I set myself limits on how long I can spend trying to find the prettiest solution before I settle for something quick and dirty. This is commonly known as timeboxing. This is particularly great when you start trying to pull at a thread in the codebase, but you don’t know at the start where that thread is going to end up. This typically involves deciding upfront how much time you want to invest in exploring a strategy, and then reevaluating that strategy once that time is up.

The other side of this is post-hoc timeboxing, where, after a few hours of exploring a rabbit hole, you decide whether it’s likely to lead anywhere useful, or whether it’s worth giving up for the time being. (Note that this comes back to the first point in this article, about using Git to make convenient checkpoints so you’ve got something to come back to!)

When the timeboxed time is up, you might still decide to carry on going, if you feel like you’re close to the end. But you can also decide to abandon the attempt altogether, or put it on hold for later. If you put it on hold, make sure you add it to a ticket so that other people can keep track of what’s happened so far.

Conclusion

At the end of the day, you’re unlikely to get rid of legacy code in your codebases altogether. And even some of my favourite codebases to work with have had regions that were more painful to work with than others. So working with legacy code is definitely a skill that is worth getting comfortable with. I hope these tips have been useful, and I encourage you to share your own favourite tips that you’ve found useful.