what you should watch for in code reviews?

Code review (sometimes referred to as peer review) is a software quality assurance activity in which one or several people check a program mainly by viewing and reading parts of its source code, and they do so after implementation or as an interruption of implementation. At least one of the persons must not be the code's author. The persons performing the checking, excluding the author, are called "reviewers".

This is how Wikipedia defines Code review. In simpler words code review means you implement it but if you think it's gonna go to production without my review, you're wrrrronggggg! I need to approve it first! 😈

Based on the team, the code review might include a review from different team members. For example in some teams as long as another engineer on the team review your code and approve it, it's enough and it can be merged to master (the main branch for the software code). In some other teams, it needs approval from more than one engineer and maybe even approval from QAs as well after testing your branch on dev environments (staging environments).

This is a process that most of the software teams do (and if they don't they should, ASAP!). It has a huge impact on the quality of the software, this is something that everybody knows and kinda is obvious. But there are none technical things as well that I'd like to talk about in this post. Things that maybe you should watch for and talk to your team about those! There are metrics you can extract from code reviews that can improve your team performance. If you've never done a Code Review, you can start with this guide from Google engineering team.

Is PR(pull request) reviews the priority for your team?

If not talk to them tomorrow! I found two main reasons why it shoudl be the priority to review the PRs (except for Critical Production Bugs).

1- Late reviews can cause blocking another part of the software and makes deliveries slower. For example, let's say the team is working on Feature A and three developers are working on it. If Alex is done with his part and John needs Alex changes to implement his part, then John is gonna be blocked untile Alex changes are approved and merged in the Feature branch. So to prevent this sort of blocking we should make sure that we review the PRs and merge it as soon as possible to make sure nobody is blocked because our changes are not merged yet.

2- Context Switch! To me, this is the most important reason why you have to review the PRs as soon as possible. Our brain is not powerful in switching context! (At least mine is not, and my friends, and everybody who I worked with in my whole career 🧠) It's Monday you're working on Feature A and you open the PR end of Monday and you start working on Feature B on Tuesday and nobody review your changes untile Friday and now there is a bunch of comments on your PR! and you're like 🤷🏻‍♂️ It's gonna take more time on Friday to remember what you were doing on that branch in compare to Tuesday! Now imagine it's not only one open PR, but there are also multi PRs opened by you!

So the first thing you should watch for is how long does it take for the team to start reviewing PRs? If it's more than a day then I'd suggest talking to the team. Try to minimize how long does it take for your team to start reviewing PRs.

What is the TTR rate in your team?!(Time To Resolve)

This is another important thing in code review. Do you remember switching context from the first part? Well, it's the same here. The longer the PR being opened, the harder to remember the context. Alex opens a PR on Monday and John puts a couple of comments on his PR 1 hour after that. He has a couple of questions. Alex doesn't respond to his comments until Friday. Now John got a notification that there are some replies to his comments, but it takes more time for him to remember why he put those comments in the first place! Even worse than that, probably he doesn't remember and he just gives a 👍 and the branch gets merged. Make sure that the conversations on PRs are going fast. If it takes one day for your team to respond to each other comments, there is a problem. It should be their priority.

The language is important! 🥊

This is probably something every developer has something to say about it! A story from a Sr. Colleague who made him cry on his first PR 😭. A Sr. Developer reviewed a PR of a Junior developer and the Junior developer found he hates Software, code, engineers, life and everything 🤮 It doesn't matter how bad is the implementation, you should always respect the developer who opened the PR. Respect should be the mandatory thing in PR reviews.

• Respect, Respect, Respect
• Be open to suggestions, and different opinions
• Always there is a chance you're wrong about your opinion, use I think always
• If you don't understand something ask questions
• Don't put vague comments, provide details, references
• Don't put comments only on things you think doesn't work if you like something say it!
• Don't overwhelm your colleague, maybe you should talk to him in person, maybe you should let it go and provide some training/mentorship later

Compare these two comments: Good comment:

Nice! A small comment here, I think you could use package A to implement it. I think your implementation doesn't support all the browsers. Here is a link to the browsers that don't support this API: blahblah.com Let me know if you'd like to go over this package together, I used it before😊

Bad Comment:

WTF?!!! There is a package for it and you reinvented the wheeel!!!😐

Good Comment:

Ow!! Such a smart way to validate it, good job👏

Check the team conversations on PR reviews, do you see some signs of conflicts in the language they use on PR reviews? Maybe you should talk to them and make sure they understand each other and respect each other. Remember The language in the code review shows the culture of your team!

Sometimes talking in person is better

Some teams think the code review means putting comments only on the PR😐Encourage your team to chat about PRs if there is something that is not easy to understand! It also helps your culture, they get closer to each other when they talk in person!

Sr. engineers can suggest training based on code reviews

This is something I found very useful. The code review is not the best place to teach a Junior some more advanced concepts and patterns. Ask your Sr developers to make a list of the things they think a Junior developer needs to learn based on her code style and then make sure you provide enough mentorship to her to learn those topics. Nothing is worse than overwhelming a junior developer with advanced topics on a PR review

If you're a manager you should always track what's going on in code reviews. A lot of conversations happen on the reviews and those are a good source to udnerstand the culture of your team and make plans to improve it. Some tools can help you to check some of these metrics in your team. One of these tools is GitPrime. Personally, I never used it, but I like their product.

It doesn't matter you use some tools or randomly you check the PRs, make sure the Code reviews process and the language used in those reviews are always under your radar.