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)
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.
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
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.
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."
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 :)
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.
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.
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!
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.
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.