mgreiler / code-review-checklist

This code review checklist helps you be a more effective and efficient code reviewer.

Hi there 👋

I'm a code review expert and trainer. I work with engineering teams all over the world to make code reviews their super power.

• 🔭 I’m currently working on awesomcecodereviews.com, a learning hub for code reviews.
• 🌱 I’m also writing about code reviews here.
• 👯 I’m the host of the Software Engineering Unlocked Podcast.
• ⚡ Oh, and of course, you can find my code review workshops here.
• 💬 Ask me about empirical software engineering.
• 📫 How to reach me: michaela [at] michaelagreiler [dot] com
• 😄 Pronouns: she/her

🎙️ Latest Podcast Episodes

• Relationships Ruin Your Code Reviews
• Measuring developer experience
• From DevOps to Platform engineering
• The journey of a self-taught developer
• Do code reviews frustrate developers?

Awesome Code Review Posts

• Best AI Code Review Tools 2024
• 13 Best Automated Code Review Tools
• Bottom-Up Code Review
• Code Review Techniques
• 10 Best Code Review Techniques

📩 Latest Blog Posts

• A Year in Review – 2023
• Measuring the Impact of Developer Experience
• Two years in Review: 2021 and 2020
• Security Code Review Checklist to find Security Vulnerabilities
• Build your own “intelligent” code review reminder

code-review-checklist's Issues

Ambiguity

There is an ambiguity in the first item under Implementation:

• Does this code change do what it is supposed to do?

I started parsing this sentence by interpreting 'change' as a verb (and then the following word 'do' makes no sense, and my first thought was that 'do' should be deleted). While writing this issue, I saw that you can also interpret 'change' as a noun. But this also does not make sense to me, because the change itself is not doing anything, the code as a whole is. I associate 'code doing something' with running the code, i.e., its execution; a code change cannot be executed in itself. But a code change can have a purpose, and that purpose can be accomplished/achieved.

Suggestion: Change the word 'do' into 'accomplish' or 'achieve' (twice).

It would be good for the reader of this list to understand the setting that you have in mind better. I started reading it with the review of a piece of (new) code in mind, rather than reviewing a 'code change' (a delta), where the reviewer is given two pieces of code (old and new/changed), the 'change' being the difference between old and new. Some items are explicitly formulated to apply to the second scenario, whereas others are more open-ended in their formulation.

This raises the question whether reviewing some code (without having some previous version to look at) and reviewing a code change are different things. You may wish to review the checklist with this in the back of your mind.

Header Typo

Should last header be "expert's opinion" or "experts' opinion"?

Typo

Under Readability an item inadvertently got split across two checkboxes:

• Could some comments be removed by making
• the code itself more readable?

Observation

I miss the heading Robustness, though some of the relevant items can be found under other headings. What do you think?

Missing words

Under "Testing and Testability":

"Has the code change"

I would be happy to contribute if that would be helpful. One idea: I recently learned that one can make readme sections collapsible now, so that they are easy to show / hide when people want to move on to the next section.