blogcontent/blog/content/posts/code-review-on-paper.md

title	date	tags
Code Review on Paper	2022-10-29T13:00:36-07:00	[politics real-life]

A day or so ago, news broke that engineers at Twitter - newly-owned by Phony Stark - were being asked to "print out their last 30 to 60 days of code, so they could show it to Elon Musk himself". We saw evidence of this from Twitter employees themselves:

This is ridiculous for a number of reasons, which I'll describe briefly. But it's more interesting because of the reactions to it.

Wait - you don't like Code Review?

First off, let me be clear. Code Review itself - the process of submitting prospective code changes for comment and approval before they are merged into the codebase - is unequivocally a good thing.

• Most obviously, it gets more perspectives on the code to catch potential issues or suggest improvements¹
• It subconsciously prompts you to work at a higher quality, because you know you'll have to justify any shortcuts or corner-cutting during review.
• An often-overlooked advantage is that it spreads awareness of what other people are working on and what's changing in the codebase, and gives you a chance to see techniques or approaches that other people are using. This is why I push back on the idea that code can only be someone of equal or higher level than the author - while a junior engineer is probably unlikely to notice issues with a senior's code, they will still benefit from the experience of reading and understanding it!
  • This also acts a legibility filter - if there is insufficient description in the commit message to explain or justify the change, or if it uses unnecessarily² "clever" code, a junior is much more likely to notice that than a senior³. Remember that code is written only once, but might need to be read many times over the course of its lifetime, and you can't guarantee that the person who wrote it will be the person who wrote it.

There are certainly issues that arise during the practice of Code Review, but in my experience they're usually process problems rather than problems with the practice itself⁴. E.g. the response to "people aren't doing Code Reviews, so we're not able to ship any changes" shouldn't be to remove Code Reviews, it should be to understand why people aren't doing Code Reviews⁵!

OK, so why is this a bad idea?

Leaving aside the absolute nonsense of requiring a printout rather than using existing online Code Review tools⁶ - the idea of reviewing 50 pages of code with a single person is just mind-boggling. With an estimated 7,500 employees, and generously assuming that the Elongated Muskrat can read, understand, and discuss one page a minute (without breaks for weak human needs like food, sleep, or hygiene), and maintain focus throughout that time, that's 260 days, or nearly 3/4 of a year, that would be spent solely on this process. Not exactly the best use of this hyper-genius' time.

But wait - we said elsewhere that a big benefit of Code Review is in knowledge-sharing? Maybe the 1:1 bottleneck was miscommunicated, and this is in fact a way for incoming engineering leaders to get up-to-speed on the state of the codebase? Well, if that were the case, then focusing on the last 1-2 months of changes is a terrible idea. That will only show what's changed over that time period, when new leaders would want to get high-level architectural overviews, descriptions of key components, processes, and user flows, and discussion of pain points and planned improvements - all of which can be accomplished without reading a single line of code. Trawling through recent commits alone won't get you much useful insight; and if it does, it will be by spawning incidental tangential conversations ("Why did you have to change the timeout on the Frobulator? // Oh, so there's this interesting component interaction we have to work around...").

The only reasonable interpretation is that this is not a means to understand the codebase, but a way to evaluate the authors in advance of making cuts (or even a tactic to get people to quit). This is also deeply flawed, because "reading the code you committed" is a terrible metric for impact⁷. Engineers - especially senior engineers, whose impact is most important to judge - have impact in many ways beyond merely typing code. Oncall, mentoring, researching, prototyping, and pair-programming are all activities that don't result in commits, to say nothing of attending meetings⁸. Most importantly, writing and presenting documents is an integral part of having broad impact - "Principal Engineer is where you swap your IDE for Microsoft Word". Judging engineers on their recent code contributions is an effective way to insult the high-performers, leaving only the less-impactful employees to keep the company running. I'm sure this will go just fine.

Why are the reactions interesting?

I found some reactions to the tweet fascinating. Note that there's no context or sentiment provided in the original tweet, so readers are free to interpret it however they wish. Many - as I did - laughingly shrugged at how Kafka-esque the whole process is. Some, however, felt the need to defend the practice, asserting that engineers should be willing and able to demonstrate their value to the company, and that anyone who was unwilling or unable to do so was probably dead weight.

This is an interesting perspective, because it focuses solely on the personal experience of a policy without considering its broader implications. If someone asked me to print off my changes and describe their value in order to justify my employment, I wouldn't be offended because it was belittling - I'd be offended because it was a wasteful and inefficient way to accomplish the stated goal. By assuming that the original tweeter is offended at the insult rather than the inefficiency, these Musk-defenders are projecting that people only oppose policies that harm them personally and that prevent them from fitting in or being rewarded, rather than opposing policies that are seen to be harmful or counter-productive even when one can benefit from them.

I wonder if there might be a correlation with one's personal politics?

---

1. In fact, there is some evidence that Code Reviews are much better suited to knowledge sharing and suggestion of improvements than in avoiding defects. ↩︎

2. The use of "unnecessarily" was intentional, here - code should be as simple as possible to achieve the desired goal, but no simpler! As the linked StackOverflow answers explore, "I am not familiar with language feature X" is not a good reason not to use that feature. We should keep learning and expanding our knowledge of techniques and approaches! "Clever" code isn't merely advanced code, it's code which relies on unexpected or unusual behaviour, which breaks with ordinary coding style in a way that cannot be easily understood, and which provides no significant benefit for doing so (other than - if we're being honest - making the author feel smart). ↩︎

3. Cultivating a psychologically safe space in which a junior feels safe to say that they don't understand a particular change is a significant task which requires deliberate action. ↩︎

4. I've seen claims that Pair Programming can effectively replace Code Reviews - it still spreads knowledge and provides opportunity for suggestions and learning, and gets two pairs of eyes on the problem to catch issues, while having the advantage of faster unblocking (you have a person right there for help, instead of banging your head against an issue for ages before finally asking for help) and less "waiting around" for code reviews. That might well be true! I've never worked on a team that executes Pair Programming well, I'd love to try it one day. I'd probably still want a review step in this process to ensure that the commit message is complete and informative to an outsider with no context, but they probably wouldn't need to do a deep dive on the actual code if it's been pair-written. ↩︎

5. My first bets would be - because changes are too large, which makes reviewing them feel like a pain, and because people aren't describing their changes well-enough for reviewers to have context (which, again, often occurs because changes are large). In these cases - the fact that Code Reviewers are painful is a symptom of a deeper issue (changes that are too large - prefer Many More Much Smaller Steps!), and so the response should be to address the underlying issue rather than to mask the symptom. ↩︎

6. Shocked that Elongated Muskrat isn't as green as you think? Not only are EVs not as green as you might think, but cars in general are just an astonishingly wasteful form of transport when compared with public transit. Only one person can get away with printing out their code, and she wouldn't touch Twitter with a bargepole. ↩︎

7. I'm being generous, here, and assuming that the code will actually be read, understood, and evaluated, rather than simply counted... ↩︎

8. Meetings are more valuable than most engineers think, when run well (which is rare)! One day I'll blog about that... ↩︎