Overview

As developers design and code, they naturally get biased looking at their own code, hence, getting a pair of fresh eyes to get feedback is worthwhile. Code reviews are not easy, but you can firmly believe that the tool can improve your code.

Goals

The goal of a code review is to improve code for your group. Any step in this direction is great, and it is not a "developer vs. code reviewer" debate.

• Code should be designed to be taken over

  • Hogging a code module for the sake of job security is not job security

  • Your objective should be to write code such that you can go on do more beautiful things

  • Do not write @author me

    • If consultation with an author is required, then there is a problem with code clarity

    • Source control should track the author anyway

• Code should be optimized for the code reader

  • Do not play tricks, and do not try to be clever

  • You should not desire for other people to come to you to ask questions for clarity

• Code review should not be a discussion about code format

  • Discussing the { braces, or spaces between if() is not the purpose

  • Use auto code-formatters, such as clang-format

Problems

One of the fundamental problems is that written speech is difficult. It is difficult to interpret emotions, and easy to misinterpret the questions and concerns of another person on a code review.

Another problem with the code reviews is that a high percentage of the time the reviewers are focusing on whitespace issues and naming standards. This is not what a code review is intended for. Most of these need to be addressed by automated tools. For example, at Sibros, we have tools that would auto-format the code to comply with a lot of coding standards to take this tedious work out of the picture. We also have tools that prevent a commit if coding standard is not quite right. Well, this article is not meant to sell you this tool, and the message is that focus on the meat, not on the whitespace appearance of the code because this needs to be addressed by a tool automatically.

Being a good code reviewer is a skill that sometimes only experience can build. I was asked once from a young engineer about why he could not come up with same code review findings that I uncovered, and unfortunately I did not have a magic bullet to offer. I did, however, advise that that I had been doing code reviews for many years. And so do not take it harshly if your early reviews are not as great as they can be.

Usual Pitfalls

The natural instincts of a developer may lead to:

• Developer to defend their code

• Developer to think that others' are attacking the code

• Developer to assume that others' are not smart enough

Think Positively

Code reviews provide un-biased feedback. When a developer designs code, there is a natural progression to the developer being biased by their own code. So taking the feedback very positively is going to help improve the code. Sure, it will take a little bit of effort to make changes and turn things around, but it would be worthwhile.

If there is code review feedback, then there is definitely something you as a developer can improve, and improvement should never be a debate. Try to give in most of the time because ultimately it will help improve your own code.

Tips

• Digest all the feedback

  • Do not react immediately

  • Read all of the comments once, and take a break

• Try the 80/20 rule

  • Try to address 80% of the code review comments, and defend 20% (with good reasons)

  • It is much easier to say resolved rather than argue and go back and forth

• Review in person

  • Sometimes people do not communicate well over written speech

  • And generally, people are much more comfortable discussing things in person

• Address all feedback with comments like addressed or fixed or not going to address because ..., as opposed to just using the resolve feature on GitLab. This way, the code reviewer knows that their feedback has been addressed, and what action was taken.

Become a Good Code Reviewer

As important it is to get reviews on your own code, it is equally important to leave feedback for others. Unfortunately, being a good code reviewer can take some time, and it is an acquired skill. You can create a plan, and participate in more and more code reviews to become good at it.

• Begin to look at open Merge Requests (or Pull Requests)

  • Ask developers to add you to relevant reviews

  • Be pro-active to spot reviews that are relevant to you, such as simply having the knowledge of a particular language

• Begin to leave feedback

  • Even if the feedback is naive, start by leaving some comments and get away from shyness

Once you get over the hesitation of leaving feedback, you will learn to acquire the important skill of being a good code reviewer. You may experience going through the following:

• Initial feedback may be minor things, such as:

  • Choice of variable names

  • Code format issues

• Eventually, you will migrate to:

  • Spotting design issues

  • Providing better implementation choices

Code Review Categories

• Is the intent of the API at the header file clear?

• Functions

  • Should be small

  • Code should be modular

  • Does the function name match the actual code of the function?

• Module review

  • Can the module be broken up?

  • Any duplicated code? (DRY violation)

  • Can we re-use any existing code?

• Unit-test review

  • One type of test per test function

  • Minimal “setup”, and maximum code-reuse

Code Approval's Responsibility

Beyond the code author, there is an inherent responsibility of domain owners to ensure that the code review objectives are met prior to giving approval. Also, do not forget the Sibros' steps to writing quality software.

• Requirements assessment and updating or adding appropriate requirement

  • Did we really build what we wanted?

  • Have the requirements been updated?

• Review design and API interface

  • Is the header file or class interfaces correct?

  • Is there any possible memory leak or an awkward code interface?

• Review the code

  • Stylistic changes should not be the focus, although you should point that out

  • Does the code address the requirements?

  • Is the code expressive and otherwise follows the coding standards?

• Review the unit-test code

  • Do the tests hit the requirement?

  • Do they leave a quality barrier behind in case someone alters the API interface being added?

References

Code Review Etiquette