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:
- Are there any failing tests? If yes, why are they failing?
- Are there any extra files included in the PR that shouldn't be there?
- Can I checkout the branch without any errors?
- Was any relevant documentation updated to reflect the changes?
- Were new packages or dependencies added to the project? If yes, why were they added? Are they needed?
- Is the HTML valid, semantic, and accessible?
- Does the CSS follow the project standards? Are all viewport widths accounted for in the implementation? What about cross-browser compatibility?
- 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:
- My code has mistakes and often can be improved, and
- 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?
Top comments (27)
This post is actually useful.
I had the opportunity of getting started to one team in code reviews.
I have two tips more to share them with everyone, which can helpful.
1 Priorities,
We had priorities with kind of issues such as security, performance, unreadable code, dry when it was needed, clean code we followed some patterns. This approach helped us so much.
2 Culture
Explain and make aware to your team the importance of this. Safety principle helps us feel safe when we make mistakes and learn from them. This principles must be permeated in all your team.
I'm glad you found it useful! Setting priorities for dealing with PRs is a great idea, and creating a culture where people feel comfortable sharing their code and receiving feedback is absolutely critical.
Great Article 🎉
Thought I'd add, for step #5 use a pull-request checklist in every repository that encapsulates all those various items. That is a good list for a nodejs/react library. I'd customize for each repo dependent on languages. Lastly, make sure there is a section considering security and risk, again these should be repo dependent.
Thank you so much!
That is an excellent addition! And I'll admit, not something I had even thought about. But a checklist included in the repo is a great way to make sure you're not forgetting any of the steps. You could do the same to keep track of the required components of a pull request too.
put it in
.github/PULL_REQUEST_TEMPLATE.mdfor it to auto-populate. 😄Great post!
I'm still trying to get better at #6 too. I keep thinking that I can review PR when I'm taking a break from my main task. How do you normally dedicate time to reviewing PR? any particular time or the whole day?
Thank you so much!
Setting aside time is definitely a struggle, and more than once I ended up working OT because I was trying to get my own work & reviews done at the same time. The best thing I've learned to do is treat reviews like their own dedicated task (because they are!), which means when I give time estimates for how long it will take me to deliver a project, I have to take into account how long I think reviews will take. So my time estimates are longer now than they used to be, but I'm fortunate that my manager is very understanding as to why that is.
I also will block off time on my calendar so I can't be booked for meetings or other tasks. The amount of time depends on the project. When we're building a website with a lot of interdependent parts, I will set aside larger chunks of time more frequently to do reviews so I'm not holding up the process. If our timeline is a little more flexible, I will do shorter periods of time (1 or 2 hours). I also prefer to do reviewing in the morning when my brain is still fresh, and I've found it puts me in a good place to start writing my own code when I'm finished because I'm more aware of good syntax, logic, etc.
I'm going to give that morning pr review routine a try. Thanks !
Good luck!
I've always believed in having my code reviewed by my team. If they can question that, it keeps me on my toes in all decisions, because I should be reviewing every important decision with the same rigour.
I think it's also important to make the comments clear and actionable. "This logic is vague" doesn't really tell me what to do to improve it. There's an opportunity to provide an example "would it be clearer like this ..." or suggest a straightforward enhancement "nested ternary expressions are hard to follow. This would be clearer if..."
I completely agree! Providing clear comments that are actionable is a must. We can't expect others to read our minds, and they obviously don't think it's vague/confusing/etc. because they wrote it and submitted it. I've found that I have started to write clearer code since starting reviews because I get called out on things like that, so now when I write it, I'm thinking about if others would be able to understand it from the get-go.
Wow, it's a really awesome article :) I like the part of everyone reviews especially by allowing 2 people with junior developers to look at your code to become better at answering your logic and write better code.
Thanks so much! It's been a really great practice. It gets them involved with the code review process from the perspective of a reviewer, which in turn leads to them writing better code on the first try (because they learn what to look for). Having to explain my choices and reconsider things I took for granted was an excellent bonus to implementing this rule, and it's made me change how I do a few things after realizing they could be done better.
If it is "them", get a team lead and HR involved. Don't resolve personnel issues on your own.
Of course, if there are larger issues/conflict with that particular teammate, you should absolutely escalate to higher ups and HR as appropriate. My perspective on this tip is meant to avoid generating negative feelings around code reviews by making it about the code and not the individual. Obviously if those feelings exist, there's a larger problem to be addressed first. :)
Sorry, I didn't mean to imply there's anything wrong with the tip at all. It's spot on. I meant only to add to it, to cover the uncommon case where it truly is a problem with the person.
Great read. Thank you
Thank you!
Thanks! Great post!
I just recently started transitioning to a "code reviewer" position so a lot of your pointers really ring true for me. I'm really grateful for #2!
Thank you! Good luck with your new position! It takes some time to figure out what works for you, but it's a skill that definitely improves with practice. Glad that some of my lessons learned could be helpful!
Excellent points! I just linked back to this article in the comments of one of my own on this topic...
10 Principles of a Good Code Review
Jason C. McDonald
Thanks so much! I remember reading your 10 principles back when I first was getting started doing code reviews and found your guidance extremely helpful in making sure I was spending my review time wisely. :)
Wow, it's awesome to know my article was helpful to you! I'm certainly saving yours for sharing with anyone new to the process.
Excellent post! Excellent! Thanks, @Jacque!
Thanks! Glad you enjoyed it. :)