code review: ideas and best practices

This blog post originated as a discussion I and other junior engineers had at my previous workplace in 2023. I took notes and wrote it up into a small guide.

Suggested process

• Does the project build? If you’re not sure about the build on GitHub actions, try to build locally!

• Go into Files Changed, look for unexpected changes, unfamiliar things (ask questions about things that don’t look right)…

• Go back to the user story and reread it, so you know what you’re reviewing and know what changes to expect.

• TIP: use the “Viewed” checkbox on GitHub to keep track of what you’ve reviewed on each iteration.

• Notes on imports and plugins: Generally, trust that import changes will be fine. If someone’s moved the Surefire (and possibly Failsafe) plugin from the pom file, only some tests will run. All test classes ending in IT only run with Failsafe enabled.

• How effective is the communication of the code? Is it easy to understand? Leave a comment (by selecting line numbers), and don’t be afraid to call people out on it.

• However, limit the amount of comments, pick your battles, make sure your comments are useful. If something isn’t clear, weigh up how much you care about it and use that as benchmark to decide whether to comment on it or not.

• Best practice is that if there’s commented-out code, make a request-changes comment to delete it:   

<modules>
        <module>node</module>
        <module>bot</module>
    </modules>
<!--    <dependencies>-->
<!--        <dependency>-->
<!--            <groupId>junit</groupId>-->
<!--            <artifactId>junit</artifactId>-->
<!--        </dependency>-->
<!--    </dependencies>-->
</project>

• Finish review and submit it as a comment/request changes/approve. Approve generally means it’s ready to merge.

• When we do reach the merging stage, use squash and merge function

Review checklist examples

Example 1

• Are there any obvious logic errors in the code?

• Looking at the requirements, are all cases fully implemented?

• Are the new automated tests sufficient for the new code? Do existing automated tests need to be rewritten to account for changes in the code?

• Does the new code conform to existing style guidelines?

from “Why code reviews matter (and actually save time!)”, Atlassian

Example 2

• Design: Is the code well-designed and appropriate for your system?

• Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?

• Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?

• Tests: Does the code have correct and well-designed automated tests?

• Naming: Did the developer choose clear names for variables, classes, methods, etc.?

• Comments: Are the comments clear and useful?

• Style: Does the code follow our style guides?

• Documentation: Did the developer also update relevant documentation?

from “Code review”, Google Engineering Practices Documentation

Best practices & advice

• Anyone can review! Even if the person who wrote the code is more senior. An extra pair of eyes is never a bad thing.

• If it’s your code which is being reviewed, do not stress. An extra pair of eyes is never a bad thing.

Further reading:

• “Code reviews”, Atlassian

• “About pull request merges”, GitHub

• “What is a code review?”, GitLab

• “Code review”, Google Engineering Practices Documentation

• “How to do a code review”, Google Engineering Practices Documentation

• “Awesome Code Review” GitHub repo by John Barton

• “A code review checklist prevents stupid mistakes”, Blaine Osepchuk