fatos bytyqi 535528 unsplash

Code Review – Good, Bad and the Ugly

Let’s talk about code review process. What makes a good code review? What are the best practices? How to prevent bugs from ending up in your production? What can reviewer do to make a positive impact on pull request?

What is a review in its essence?

It is a process, which has a goal to assure the quality of a delivery. In this specific case, team is reviewing a quality of code. This is accomplished by one or more developers examining the code which their colleagues wrote.

code review process wtf
Code Quality Measurement – Medium, unknown source

The quality of code review is hard to measure. However a good code review should be able to accomplish several items.

  • Find potential problems/bug in code and provide ideas on how to handle it.
  • Improve structure of the code if possible (utilize proper software design patterns, refactor and better segmentation, etc.)
  • Check if the feature actually work, while trying to find additional test (and edge cases) to check if the code would work in specific situations. 

What are the best code review practices?

Firstly, I did a quick internet research for this topic. I have found many interesting blog posts, but some of my favorites are best coding practices, 10 principles of good review and 7 steps to a complete code review.

Secondly, I have compared these posts with my experiences as a developer and reviewer.

Finally, I managed to compile a list of common sense rules and checks that should be done during a code review.

Common-sense rules and checks of a code review

Before starting with a code review, we should consider several important ugly truths:

  • Reviews won’t happen in a minute, it takes time to do it properly.
    • Team should plan proper time block, depending of a complexity of review.
  • Ticket planning should include time needed for a proper review.
    • The team should be aware of something called Definition of Done.
    • This is a list of things that should be completed before ticket is considered as finished.
    • Review tasks should be part of DOD and review time should add upon that ticket.
  • A team should calculate cumulative expected time for this ticket.
    • If 3 developers spend 2 hours that is actually 6 hours for code review, which is 1 man/day of sprint.
  • Original developer should test reviewed code before inviting others to review it.
    • This is important, because if will save everyone’s time. Developers will always make bugs.
    • Only question is when will the team discover these bugs.
  • Managers will not always have understanding for time needed to do proper review.
    • It is up to developers and reviewers to communicate this properly.
    • Other team members will not always have understanding for long or complicated reviews.
  • People make mistakes and that is completely all right. Bugs might appear in code, even after a good review.
First Time Managers
Bodith – First Time Managers

Furthermore, we are aware that different reviews can have different weight. For example, some reviews will be easy, while some of them will be hard.

Let’s try to group these code reviews in categories.

Categories of code reviews 

Determining a type of review early is extremely important because it provides us with information on how to handle it. Furthermore, we should be able to compare our capabilities to properly examine provided code.

Finally, if the review is too challenging we might decide to invite another reviewer or even ask to be additionally educated on technical topics from that review.

pyramid of doom

Code review is also called the pull request, I am using both terminologies in this article, depending of sources that I am referring.

Classifying by urgency

Ben Balter, a Senior Manager of Product Management at GitHub, wrote a fantastic post The six types of pull requests you see on GitHub . This post classifies pull requests as:

  • Just a heads up
  • Sanity check
  • Work in progress (WIP)
  • Early feedback
  • Line-by-line review
  • Pull request to pull request

Furthermore, Ben described how each of these types works and when to use it. For example “just a heads up” would be a simple (one-liner) pull request where author don’t need review.

On the other hand, line-by-line is pull request which is used when product is ready to be shipped.

There is no denying that pairing is a highly visible cost to management: “Two developers at one workstation? No way! We already pay them a fortune!” – Paul Blair,
The Unnoticed Costs of Code Review by Pull Request

Classifying by size and familiarity

Firstly, when we examine new pull request, we need check number of changed lines.

If pull request has more than 500 lines author of that request should consider splitting it into multiple parts or find a way get involved in code review process.

Furthermore, reviewers should check if tests are executed for big pull requests.

Otherwise, reviewer might find critical issue in the middle of review, and then everything would go back to start.

Classifying by impact

There are two ways to consider impact of a pull request:

  • What positive and fantastic things will happen when we launch this new cool feature?
  • What s*it will hit the fan if and when something goes wrong with this cool new feature?

The second one is the important one.

When measuring impact of pull request, team should think about several things:

  1. Could there be any data corruption?
  2. Will we be able to revert the problems if they happen?
  3. How many users might be impacted by this?
  4. What kind of policies might be compromised by this? (GDPR i.e.)
  5. Is this feature related to SLA (Service Level Agreement)?

But wait, should developer or reviewer really be concerned about these items during the code review?

No.

Because these things should have been defined when ticket is created and planned. For example, during the sprint planning phase, or even earlier during sessions between product owner and management.

However, developers should not close their eyes if they notice any of the mentioned issues.

dt060211
On the other hand, don’t overthink it. – Dilbert again

Code review checklist

1. Test cases

Execute any available tests for feature which you are reviewing.

Think of manual (quick tests) which can be done. For example, if the feature changes the database connection function, simply try to connect to database. If your feature is changing BasicController component for API, just try to execute couple different API calls

2. Syntax errors

Different developers use different coding editors. Some companies have policies about this and some don’t.

Therefore, it is expected that some syntax errors might crawl up to a pull request.

What does this means?

Don’t examine pull requests only trough pull request interface (i.e. bitbucket or gitlabs). You can also check out these branches and examine them in your trusted editor.

3. Coding practices

Most of the companies have some document with coding standards or best practices.

If such document exist, pull request should include comparing the latest code with these best practices.

If there is no such document, here are some quick things to check:
– consistency in variable names
– proper comment blocks on new functions
– separation of concerns
– code should not contain any sensitive data (i.e. access tokens)
– code should not contain any demo data (except inside of the tests, or where this is really needed)
– code should be clean

4. Software design patterns

Depending on pull request and type of the change some software design patterns can be applied.

Also, if some design patterns are already inside code architecture they should be applied.

For example, adding a new function in all inherited classes of an abstract class would not make sense. Furthermore, having a strategy pattern in place and not creating a new strategy for some configuration would also be a wrong usage of that design pattern.

5. Legacy aspects

Your software will always have legacy software.
Therefore, your pull requests will affect this software somehow.
Therefore, you need to handle this. 🙂

Conclusion

The code review is extremely important part of the whole software development process. Therefore, whole team must make enough effort to properly plan time needed for this.

Pull requests differ from case to case, but teams should always be aware of impacts and potential issues.

Mistakes will happen to those who work. Code review is essentially a risk management, therefore it should be treated as such.

Good code review will improve overall quality of the code, but it will also reduce the risk related to a new feature.

If you like this post feel free to check out some of my posts from a different web sites, in guest posts section of this site. Likewise, there is a page about projects that I was involved in.

One thought on “Code Review – Good, Bad and the Ugly”
Leave a Comment

I am preparing online courses "Python for Beginners" and "Software Design Patterns"


Reserve your FREE spot by subscribing to my newsletter.
Once courses are completed you will receive entrance coupon and full access to this course.