The 3 types of code reviews

Most of us do code reviews on a daily basis, and some days we review more code than we write! Code reviewing is rarely taught so most of us have our own method of giving reviews. What I had realised over time is that there is not just 1 type of review, there are 3 types of reviews! Breaking down the code reviews into 3 types has helped me give more focused reviews more efficiently. These are the 3 types:

• Domain review
• Style review
• Performance review

You’ll notice that this mirrors the coding mentality of “make it work, make it beautiful, make it fast” and that makes sense; the review process should mirror the coding process.

Domain Review

The goal of this pass is to ensure that the code is correct; that is is behaving according to the feature specification. At this stage we would reference the requirements and any relevant business documentation to make sure that the feature in implemented correctly, well tested, and that no regressions have been introduced. This is the step where review comments would look like these:

• “This doesn’t seem to be the same as what the requirements document is asking for”
• “You might be missing a test case for this edge case, and I don’t see it being handled in the code either”
• “Is this part still backward compatible with Feature X?”
• “This formula looks suspicious, I think you might have implemented the wrong one!”

Style Review

Whereas the Domain Review is about what the code is doing, the Style Review is about how the code is written. Here we review on the readability of the code, whether the most best code construct was chosen, the appropriate library/framework has been used, and if the code was written in an idiomatic approach for the language. We may find ourselves giving comments such as:

• “Have you considered using this pattern here instead?”
• “This part should be extracted out into its own method”
• “We already have an abstraction that does all of this for you! Refer to FooClass”
• “Please give a more meaningful name to this property”
• “The spacing and line breaks are off!”
• “This section seems to be written in the style of Language A, in Language B, the idiomatic way to do this is like so”

If you are keen, I had also written more on writing fluent code, which may help with a style review too!

Performance Review

As the name suggests, we zoom in on the performance in this section. We would be reviewing on the scalability and complexity of the code, behaviour in stressed conditions, latency, and impact on server/device metrics (for example, memory usage, CPU utilisation). Comments like these belong in this section:

• “It looks like this Map will keep increasing in size, we may run into memory issues after some time”
• “Did you consider doing these operations asynchronously?”
• “This is O(n!)!!!”

An example

Let’s say we have this hypothetical requirement and this code submitted for our review:

Given an array of int, and an int k, find the count of pairs that sum to k.

int count(
   int[] arr, 
   float k
) { 
    int count = 0;
    for (int i = 0; i < arr.length; i++) 
        for (int j = i + 1; j < arr.length; i++) 
            if ((arr[i] + arr[j]) == k) 
                count++; 
    return count;
}

These are the comments we could have given during each review pass:

Pass 1: Domain review

• “The requirement states k is an int, but float was used here”
• “The nested loop should be incrementing j instead of i”
• “Add unit tests!”

Pass 2: Style review

• “The function name count isn’t very meaningful, perhaps name it countPairsSummingToK”
• “The convention here is to always add braces {} to for and if”

Pass 3: Performance review

• “This is an n-squared solution, have you considered another approach?”

Why the hassle?

Most of us go through a combination of all 3 types at the same time when reading through the submitted code, but if we were to break it down and review it in 3 passes, we get these benefits:

• There would be better focus on each pass as the context of each comment stays the same. For instance, while reviewing the code for correctness of logic, it is easy to get distracted by an out-of-place syntax and start commenting on that. By focusing on only the domain issues on the first pass, we ensure that all problems are covered with the context fresh in mind.
• The review is done in order of priority. If time runs out or we get pulled away, we would have reviewed the more important aspect of the code first.
• If there is time to do 3 passes with the code submitter (that is, review the domain, then wait for the submitter to make all changes before continuing to style and so on), it is also easier for the submitter to understand the context and overall picture of each pass. If there is no time for waiting, an alternative is to prefix each comment with [Domain], [Style] or [Performance] and let the submitter go through each phase at their own time.

References

• Make it work, make it beautiful, make it fast
• Write fluent code in Kotlin

I also write about the TryHackMe rooms (Pen Testing)!

Disclaimer: The suggestions above are of my opinion and may not be a generally-accepted best practice.