It is likely as a software engineer to receive a couple of Pull Requests for code review per weekly basis. Especially if you're working in a team environment.

So, how do you respond the moment you have to give a code review ?

Based on my experience this article describes an approach on code reviews and highlights some of the dos and don'ts.

The story goes like this...

A notification pops up!

A teammate has completed the implementation of a new feature and just sent you the PR for a code review.

Before we proceed it's good to have in mind that:

1. The author of the PR might be in a blocked state until you're done with the review, so that can perform extra changes based on the comments you left or proceed by merging to the target branch.
2. Code reviews should be treated as part of your job responsibilities and not as a chore. By doing it properly you contribute to the codebase and its quality even if you didn't write a single line of code in this new feature.

So, let's review this PR

Oh, wait! Hold your horses!

Do you have any idea what are the business requirements for this new feature ?

If the answer is No, you probably should.

Normally a PR's description should be well defined of what are the changes included. A link to the related task or bug that triggered these changes would be helpful for the reviewers.

It's not a blocker in any case but it would make more sense to review some code changes which you know why they should be there and what are the use cases they should cover.

For example, let's say that this new feature is a timer that counts down after user login. If he stays idle for 10 minutes then he should be automatically logged out for security reasons. Knowing that this is a business requirement, does the PR cover this use case? That's something that should be checked during the code review.

Having said that, it's time to move on. You could also checkout the new branch that includes the changes related to the aforementioned feature so you can take a closer look and run the app locally to see it in action.

In this step, if you come across any unexpected behavior it's definitely something that has to be given as feedback in your review comments.

Also before posting anything yet you could take a quick look at the changes to warm-up.

Now you're good to go and add your review.

Adding your review

Prefix your comments

Without being strong whether it is a good practice or not, I use to mark the comments I post with a level of importance or severity if you like.

Here are some comments for example:

[minor] Consider moving this value into a constant because it is used more than once.

[major] This event listener should be unsubscribed when the component is destroyed to avoid memory leaks.

Do you get why the above comments have been marked as minor and major respectively ?

The first comment, is a suggestion and even if the author decides to leave it as is the product wouldn't be affected somehow.

On the other hand, the major comment is a really important one and should be addressed by the author because it could affect the product's performance.

To sum up, feel free to mark your comments either as minor, normal, major, critical or low, medium, high or whatever suits you the best. Not only it will not hurt anyone but it will probably help the author to understand which of these should address with higher priority.

Do not spam

Really, don't.

There are some cases where the author followed a specific pattern multiple times in a single PR. Then, you might think that this is an anti-pattern and it should be mentioned. If you feel like this you should definitely give your feedback accordingly.

But how would you do it ?

Maybe start firing the same comment for every single occurrence of this pattern ?

The short answer is No. That could be rather annoying for the author.

You could instead leave a general comment at the end of your review explaining what is your concern and how would this be addressed.

So, instead of repetitively posting this:

Please don't use inline styles.
...
Please don't use inline styles.
...
Please don't use inline styles.

You could post one single comment at the end such as:

Please consider moving inline styles from html to dedicated css files per component to keep consistency across the codebase. Also since some styles are common for various elements we could benefit here by reusing some of these classes.

See ? Less annoying and more constructive feedback for the author.

Be aware for code duplication

Scenario A: Duplicate code within the PR

A specific block of code occurs more than once across multiple files or even in a single file. Wearing the reviewer's hat you should mention that something like this could be transformed into a function or component and be reused accordingly.

Scenario B: The PR introduces functionality that is already implemented

This is more tricky than the first case because if you aren't familiar enough with the codebase it is very difficult to catch it. But if you're that familiar it's nice to give your feedback and also help the author find the existing implementation by pointing in which file can find what's needed.

Promote reusability and testability

This also depends on your team's best practices, principles, etc.

Nevertheless, I think there is no doubt that a reusable and testable code ensures a maintainable codebase and also increases the development speed.

It's another one of your many responsibilities to point out complex blocks of code that are difficult to read or test and can be split into multiple single-purpose functions or components.

Avoid being strongly opinionated

It's totally normal and more than expected that if you were to implement this feature you could probably have followed a different approach.

This does not mean in any case that the author's approach is wrong.

Of course, you could suggest a different approach -even with a minimal example- explaining why would you do it that way but do not try to enforce it to the author to satisfy your preferences.

Going the extra mile

Apart from posting the bad things, you could also highlight the good ones.

Kudos using that function here 👍

Leave a general comment if needed to summarize your overall feedback over the PR.

Left you some comments regarding some extra error handling that could be used in certain API calls but other than that looks good to me.

Getting to the conclusion

You can do either a complete code review or just give thumbs up to get rid of it.

You can be either open-minded or strongly opinionated with different approaches.

You can be either polite or rude in your comments.

It's your call but I think that code reviews should be done with respect to the author, your team and the codebase.

Do it and you will also benefit in many different ways.

After all, your feedback matters.