DEV Community

Jenn
Jenn

Posted on

My code review checklist

I like reviewing code. I often learn new tips or tricks whenever I review. It doesn't matter if it expert or novice code, I learn from each.

Keeping track of everything can be difficult when doing reviews, so here is my personal list of things I check for.

My reviewing code checklist

  • Spelling: Spelling things correctly helps others search the code more effectively when debugging, refactoring, or writing enhancements.
  • Transposed text: If it is a content update, I double check the source material.
  • Links: Any relative links? Absolute links? SSL? The correct answer depends on the project but I always click through to check that they work.
  • Overly complex code: I do not like code golf. Writing overly elegant code just makes it more brittle and harder to support. If I can’t understand it at 3am during an incident, I want it refactored or documented thoroughly.
  • Does it meet code standards? I like code standards. If everyone knows what the requirements are, the more likely code will be accepted. Also known as, "Argue once over tabs versus spaces, decide as a team, and WRITE IT DOWN."
  • Comments: Format and length depends on the team (see code standards). Some teams prefer commit messages or READMEs over comments in code. This step could be renamed, "Is it complex or messy but documented?"
  • Security: Is this resource only for admins or editors? How is that checked? Are inputs sanitized?

Actions

  • Run the linter: Sometime people forget.
  • Run the tests: Yes, even if there is automated testing.

Testing

  • Run the code: Did the change work?
  • Check existing behavior: Does this change affect something else unrelated? I usually check only two or three key paths.

If everything looks good, I approve the pull request.

Reviewing code shouldn't be a chore. It is a great learning opportunity for everyone.

Top comments (10)

Collapse
 
sigje profile image
Jennifer Davis

Great list Jenn! If I'm reviewing the code with the person who wrote the code, I like to ask "what is this supposed to do" before I dig in. Sometimes what it's supposed to do is different from the requirements as I understand them and it also helps me review that the tests are doing what they need to do.

Collapse
 
hermetnicolas profile image
Nicolas Hermet

I like to ask "what is this supposed to do"...

Isn't it the purpose of the pull request's description (assuming you're doing pull requests) ?

I'm deeply interested in your workflows of code review with PR, for we are really struggling to achieve that part in my team

Collapse
 
geekgalgroks profile image
Jenn

The Pull Request message and the requirements from the customer or business analyst can and sometimes do differ.

People forget to document clarifying questions, branches drift from the requirements (aka "I'll just fix this while I'm here"), and before you know it the PR is bigger or completely different than what was requested.

I like to keep things small so there is less drift and have an issue for each PR so clarifying questions are documented.

Thread Thread
 
sigje profile image
Jennifer Davis

exactly! So before I even look at code, I want to have a conversation with the PR author even for just a few minutes to understand the plan. Often folks in the commit message will describe the code rather than the plan. It's easy to get caught in that trap if you just look at the description and then read the code. "Oh it's supposed to increment this value until condition. Yes, it does increment this value until this condition."

Collapse
 
vilfrim profile image
vilfrim

This might belong under code standards or comments but I'm one those who reads the commit messages and checks that the changed files are related to the message e.g. message says "Updates README.md" then why it also makes changes to index.html.
For me commit messages are important, they tell me why something was changed not how or what. Of course sometimes "fixes a typo" just fixes fixes a typo :)

Collapse
 
elvezpablo profile image
Paul

Do you find there are still debates over things like tabs and spaces with linters in place? I feel like linters have fixed that issue for the teams I've been on.

Collapse
 
geekgalgroks profile image
Jenn

Linters do help. It is easier to take feedback from an "impartial" linter on spacing than a human. But I have seen the debate continue even with linters with teams that use multiple languages or frameworks.

Most of the time it comes back up when working with old code. Do you rewrite everything to meet the new standards or just the section you are fixing? If the policy isn't explicitly written out, it can get ugly.

Collapse
 
elvezpablo profile image
Paul

Good point. While I personally don't get to attached to the formatting I have heard others express that linters feel dictatorial. I could see how a human touch when doing a code review and an agreed-upon standard as a backup helps keep the debates at bay. Thanks!

Collapse
 
silentsudo profile image
Ashish Agre

Code Formatting: one of the main I would consider, often when a new developer joins in he has his own settings, fixes 1 bug and boom you seem a lot much change in PR.

Collapse
 
geekgalgroks profile image
Jenn

That is where linters come in handy. You have the debate over code standards once and then put those rules into a linter.

It feels less mean having a computer yell at you about spacing versus a person reviewing code.