10 Lessons Learned Conducting Code Reviews

Last fall, my development team grew from 1 person (me) to 3. Proper pull requests and code reviews all of a sudden became critically important to ensure we had a cohesive codebase after multiple developers from wildly different backgrounds had worked on it.

As the Lead Developer on the team, I was the primary person responsible for conducting these reviews. Having never done this before, there was a lot for me to learn about making these reviews effective. Here are #10 tips and lessons learned from my experience doing code reviews over the past several months.

1. Always Start with a Positive

Sometimes, there’s a lot wrong with a PR: the right elements weren't used, the implemented design doesn't match the mockups, the logic doesn't make sense, etc. As a reviewer, your job is to look for these kind of mistakes and point them out to be corrected, and it can be easy to only focus on those negatives.

But nobody intentionally makes mistakes, so having those mistakes pointed out publicly can be a negative, uncomfortable experience. Writing out all those mistakes isn't fun for the reviewer either. To set a positive tone, I've learned to always start my reviews with gratitude and something I thought they did well. Even if there are a lot of issues with the code that need to be addressed, it's important to thank the person for their contribution. Code with mistakes is better than no code at all, so thank them for taking a first pass at resolving the issue.

If code isn't mentioned in a review, it is generally assumed to be good, but being active with your praise goes a long way. It builds confidence and makes people more receptive to feedback because they know you've spent time thoroughly checking their code. If their logic for X wasn't so great, but they did Y and Z well, let them know!

2. It's not "you", it's "the code"

If you've ever learned about conflict resolution, you've probably learned the importance of "I" statements. Using phrases like "I think...", "I feel..." instead of "You did..." can go a long way in resolving tension and shifting the focus from an individual to the problem itself.

The same approach should be applied to code reviews. When offering criticism, it's important to avoid the word "you". It's already difficult to accept criticism of your work, so don't make it harder by making the person feel like it's them who are wrong. For example, switching

Your logic on line #25 isn't clear.

to

The logic on line #25 isn't clear to me.

changes the tone of the comment completely. And I know which one I'd rather read when it's my code being reviewed.

3. Use Line Item Suggestions

My team uses GitHub for most of our projects, and one of my favorite features is the ability to post a comment about a specific line of code. It takes away a lot of the hassle and potential confusion that comes from writing out the specific file/line number in a larger comment. If most of your comments are single changes, this can help make a simple checklist of fixes that need to be made.

Bonus: GitHub has line item suggestions. Check out @_darrenburns "8 Productivity Tips for GitHub" for details.

If you use a different platform that doesn't offer this feature, then I highly recommend including/linking to the specific line numbers when writing your review. It might be more work for you as the reviewer, but it will help ensure those necessary changes aren't overlooked.

4. Set Project Standards Early & Tools to Help Review

We all learn how and prefer to write code differently. When I was the sole developer, I never had to worry about things like code style, naming conventions, or other project standards because however I did it was the "right" way. I realized very, very quickly when I started to review other people's code that not everyone had the same internal standards as me and that not only led to confusion in the codebase, but required a bunch of extra mental energy for me to review.

The lesson learned here is simple: set your project standards early.

Whatever those standards are, it's important that the whole team is on board with them and knows what is expected. And you need them for everything: languages being used, branch naming conventions, project organization, comment conventions, etc.

Aside: This process made me realize I had strong opinions about CSS property order, so I made a point to address that in our discussion.

Whether you develop your own standards in-house or use one of the many existing standards out there is up to you, but it's critically important that you do if you want to have any semblance of a cohesive codebase at the end of a project.

Tools to Help Review

Once you have your project standards, it's worth spending the time to set up the tools that are designed specifically to help enforce those standards. You can use a Linter to analyze your code and compare it against a set of guidelines for syntax-related standards. It will flag any errors, or can possibly even fix them for you automatically.

To catch any errors that make it into the PR (although ideally, they're caught and dealt with locally first), we've set up TravisCI to run additional checks. So if the Travis build is failing when I go to review, it gives me a place to start before diving deeper.

5. Start Broad & then Dig Into Details

There can be a lot of code in a single pull request, so in terms of actually doing the code review, I like to start broadly and work my way into the nitty-gritty. For me, that process usually looks like this:

1. Are there any failing tests? If yes, why are they failing?
2. Are there any extra files included in the PR that shouldn't be there?
3. Can I checkout the branch without any errors?
4. Was any relevant documentation updated to reflect the changes?
5. Were new packages or dependencies added to the project? If yes, why were they added? Are they needed?
6. Is the HTML valid, semantic, and accessible?
7. Does the CSS follow the project standards? Are all viewport widths accounted for in the implementation? What about cross-browser compatibility?
8. Does the JavaScript follow the project standards? Is it modular? Is it easy to follow the logic? Are edge cases accounted for? Are there stray logs in the console?

If there are a lot of issues in the first few steps, I usually end my review there and wait for those issues to be addressed before digging into the denser part of the code.

6. Establish Dedicated Review Time

This has been one of the hardest lessons for me to learn, and is something that I still actively struggle with. Doing a thorough and productive code review takes time. Even if the PR is small, it still (usually) requires checking out the branch, checking the logic, reviewing the issue, etc. When you have other competing priorities, it can be tempting to ignore that pull request or do the bare minimum when reviewing, but that isn't a good long-term solution.

My solution to this was fairly straightforward: I have a dedicated time when I do reviews. I block it on my calendar and my team knows when those times are so they know when to expect feedback. Setting expectations for your team and yourself is key.

7. Establish What You're Expecting

Do you link your pull requests to issues? Require a certain format when submitting a pull request? Have a limit to how many lines of code should be included in a single request?

If you answered yes to any of these questions, then it's important to establish those expectations up front. It saves you time as the reviewer and helps prevent your team from having to guess what it is that you're looking for. Take the time to talk to your team and figure out what works best for all of you because...

8. Everyone Reviews

On our team, everyone has the opportunity to do code reviews and everyone has their code reviewed. As the senior developer on the team, my pull requests are reviewed by the two other members before they can be merged. I do this because:

1. My code has mistakes and often can be improved, and
2. It creates space for the more junior devs to ask questions about how/why I implemented something the way that I did.

I'm a big-believer in that the best way to learn how to code well is to read well-written code, so even if there is nothing to change, it allows them to read good code examples that are relevant to the project they are working on. We frequently have good conversations about implementation in our code reviews, and it's made me more confident in my decisions when I'm able to answer questions about them.

9. Reevaluate Often

Conducting code reviews has been one big learning process for me and for our team. We've made a lot of changes along the way based on our experience. It's important to carve out time to reflect on this aspect of the project; I usually do it as a part of a broader post-mortem on a project which helps solidify code reviews as a critical part of the development process.

Ask what worked well and what doesn't, and be honest with the answers! Ask your team members what kind of feedback they found helpful or identify areas where the critique can be improved. Consider how much time you spent doing reviews and identify any patterns from it. If you were frequently commenting on the same types of issues, are there steps you can take to address it so there are less of those issues on the next project? Reflecting on these types of questions and making changes will improve the review process for everyone on the team.

10. There's more than One Solution

This one is hard. Really hard. When you are the reviewer, it can be so tempting to start refactoring the submitter's code to reflect how you would have done it. What starts out as a simple refactor can very easily turn into redoing the entire section of code to your "right" solution. But that isn't the purpose of a code review.

It's a fine line, to be sure. Sometimes the code does genuinely need to be reimagined and fully rewritten, but that should not be happening frequently. If it does, then a better approach would be to pair program before the code review stage.

But just because it isn't how you would have approached and solved the problem, doesn't mean it isn't good. When you open a pull request and start the review, you need to check your ego at the door and be open minded to considering new solutions. Use it as an opportunity to learn, not show off how smart you think you are.

Bonus: @maxwell_dev wrote a brilliant post on "It's Not About You" that addresses this.

It's been a journey learning all of this, and I'm grateful to my team for their continued patience while I've figured it out. In the nature of continued learning and reevaluating often (#9!), what are some of your tips for conducting code reviews?