Code-Review

After reviewing a lot of pull requests, I’ve settled on a simple default: if my comments are all nitpicks, suggestions, questions, or non-blocking issues, I leave them and approve the PR at the same time. Here’s some detail on how it works. But first, two clarifying questions. Why Leave Comments, If Approving? Why leave comments if I’m approving? Comments show that someone has thought about the problem and the solution, and cares about what’s happening to the code. Occasionally, they offer a chance to learn or to surface misunderstandings, assumptions, or hidden risks. ...

What is a programmer but a series of PRs (pull requests)? I optimize PRs to introduce the best code I can, be easy to review, and document my work so I can make sense of it in the future. Here are some things I always check before opening a PR. Passing Static Analyses All of the static analysis tools in my project must pass locally. On a JavaScript project, that’s an auto-formatter (Prettier), linter (ESLint), and type-checker (TypeScript). All of these run on files I touch automatically. If that’s not feasible for you, a Husky pre-commit hook will do the job. ...

Reviewing code is tricky. When I’m doing it, I’m trying to achieve a few things at once. In this post, I’d like to document the ways I try to add value via code reviews. My Background & Biases This is the second post I’ve written on this subject; here’s the first. Since 2022, my approach has shifted from strategic to tactical; I don’t prioritize design (the high-level programmatic approach) as much, which I’ll explain in a moment. ...

Have you ever seen a pull request that seems to completely explain itself? It’s a real artifact. I don’t know the project, yet I understand it. How can we get results like this on every pull request, from every developer on the team, every time? The answer is a pull request template. Templates give your team a format that’s detailed, discoverable, and predictable. In this post, I’ll share this technique and why it matters. ...

Here’s some advice about programming I’ve found useful: “Don’t ask for advice; ask for a code review.” In this post, I’d like to explore what I think this adage means. When learning programming, we think there’s a trick to it, that can be solved with advice. If I… …learn the right language, framework, or text editor …attend the best CS program, bootcamp, or workshop …master the ideal learning strategy or morning routine …I’ll have an advantage. In my mentoring of new programmers, many of the questions I answer are about topics like these. I think the answers matter much less than people might think. I stopped giving generalist career advice for a while because I felt like my answers to these questions weren’t helping people. Or that I was communicating my privileges and survivorship bias. ...

Code reviews are important on many teams. Do them well, and your code ships quickly and safely. Do them poorly, and your code ships slowly and riskily. I try to contribute good code reviews. In this post, I’ll share my process. What I Bring First, I try to be nice 😊. There’s a lot of writing about how internet comments can be perceived as less friendly than the author intended, and code reviews suffer from this. The comments are visible to your team, too, so the emotional stakes can be high. Approaching the work with curiosity, saying “thank you”, and adding emojis and GIFs, are little ways to be more human. ...

Many pull requests go through a cycle: programmer opens pull request, maintainer gives feedback, programmer makes changes, repeat until ready to merge, maintainer merges. Prior to the merge, the pull request can be messy, full of reverts, fixups, and WIP commits. In the end, those commits are noise. We can tell a better story by squashing the branch. Consider this Git branch history: * 724d574 (HEAD -> issue/38) Good to merge! * e2af5ac Refactoring the previous commit * 3e36475 Test are passing here * ea479cb Revert "Ooops" * 77aee1d Ooops * 4c7ead2 Test breaker The final commit at the top, 724d574, has been approved to merge by the maintainer. But this branch is a mess. We have a commit that’s just refactoring (e2af5ac), two commits that cancel each other out (ea479cb and 77aee1d), and one that has failing tests (4c7ead2). ...