Be better at code review!

Hello! Today, I would like to share my few rules that keep me in a frame of doing a good code review. I don't wanna just copy and paste other people code review rules and repeat obvious things like "be specific".

Context

I have seen a lot of code reviews with not related titles, without a linked issue to the kanban board, without any summary and so on.

As an author, you should keep in mind that other programmers are working on other stuff. They do not always remember what you are doing in detail. Good practice is to provide a short summary about what changes are.

Also, if I’m pushing some not casual changes I add comments to the specific line of code to make sure that everyone understands why I’m doing something.

Coding style

Mostly, each project has some coding style. I would recommend making a list with all of them and keeping in mind/taking a look before applying for a code review/being a reviewer. That should be a first point to check, if the changes are coherent with current standards in the project.

Most of such rules should be handled by tools like linter to not write dozens of comments about not sorted object keys and so on.

But of course, we have some standards that cannot be automated and for that also is the code review.

Check all of the changes at once

In my career I noticed that some programmers have a manner to start doing code review and in case of writing 1-3 comments they automatically stop checking other parts of this review.

As far as I know, this behavior is due to the prediction that the second part of the (not yet checked) changes also has errors and the author will know to change something else too.

Of course, this approach does not and will never work. Only leads to annoyance for the programmer and a waste of time:

From the reviewer perspective:

• Waste of time because reviewers have to change the context of their work frequently. Going back to the same code review several times and recalling what it was about

From the author perspective:

• Cannot have any feeling about how long the code reviews will take. Anyone cannot predict it because any other programmer can come back here and check other parts of the changes and find other mistakes.
• Author does not know if part of the changes without any comments is good or just haven't been checked yet.
• Author may be annoyed if the code review is like ping-pong. The author has resolved all comments, waits for approval, and only gets more comments.
• Author needs to do a lot of context switching to come back to code review and resolve/fix new comments. Overall it will take more time than resolving everything at once.

If you checked changes, leave approval

I'm often witness situations where reviewer has written some comments, they are resolved/fixed by the author and that’s it. There is no mark that needs any more changes and CR does not have an approval.

Keep in mind that a lack of response blocks author and tester future work. If you checked code, and your threads are resolved, mark the code review as approved to do not block the workflow.

Focus on important stuff

That’s code, we have rules but some of the approaches are flexible, neither good nor bad. It means that we should not force only a different approach that does not change anything. You should discuss in the team which approach you will follow (and add it to coding style rules).

But! There are some rules that are clearly irrelevant. Do not waste CR author time fixing insignificant stuff. Do not require to move function from one place to another, which changes nothing. Do not ask for renaming a variable name from the good one to another good one. If something is important - should be in coding style rules or be cared for in any other way.

Tests

Check if the most crucial changes have been covered by tests (unit/e2e/integration), if no, ask the author why. I think that tests are the most important of all above, because also thanks to tests other programmers can easily catch edgy cases of solutions, learn how it works and the most important - we really know that changes work like they should.

Logic

Coding style and tests are not everything. The most important thing is logic - especially business logic. Keep in mind that this is the heart of the program. Logic should be as simple as it could be. So let's dig into it and try to understand it in a reasonable amount of time. If something is not clear, ask for clarification or better - code change to make the logic easier to read. Remember that you or your colleagues will return to this part of the code maybe after a year, two or three. The logic should still be understandable to keep it going.

Last words

That's all about the code review. Of course I purposely didn't mention things like checking the code carefully, being constructive and so on - I think that everyone of you already knows about that ;) See ya in the future posts! I hope you will find it helpful. And thanks for reading! If you enjoyed it, I will be very pleased if you follow my GH account: https://github.com/RafalKostecki