Code review goals and merits, and why you might not always need to do it

I want to talk about code reviews.

Pretty much every job I have ever had as a software developer and every client I have worked with has had some kind of code review process.

A simple search for the term code review yields many results, so what do I think I can add to the conversation?

Well, most of these search results either relate to specific things we should be doing or looking for in a code review, or are for tooling to supposedly allow us to review code better, or to actually force us to do a review.

Most organisations though, aren’t talking about why we do code reviews. I am a firm believer in equipping myself with a set of tools as a developer, and then having the ability to make intelligent selection of the right set of tools to use in a given situation, given a set of goals I am trying to meet. Tools such as TDD, unit testing, integration testing, pair programming, and, yes, code reviews.

Moat organisations and development departments are keen to put a code review step in place, probably as a mandatory step in Jira in between development and test.

After all, it’s pretty hard for anyone to argue that code review is a bad thing.

My problem though is that this can often leave us with a process that just feels good. It feels like we are doing the right thing by having a mandatory review step.

But, if we don’t agree on what purpose this step in our development process is serving, then how do we know it is serving this purpose, and therefore a valuable thing to be doing?

And, as a reviewer, how do I know what I should be checking?

Should I be running the code?

Just inspecting the code?

And what should I look for when I do inspect it?

If I do have any questions or comments, then does the original developer have to address all of them?

So, what is the point?

I would like to start by working out what some of the most common reasons people cite are for doing code reviews, and use this as a starting point.

Once we have this, I would then like to look at a number of different tools that we might use to achieve those cited benefits.

I feel that code review is almost always used dogmatically, without a shared purpose in mind. If we equip ourselves with a shared view on what we are setting out to achieve, then we may find that code review isn’t always the right tool.

And when it is the right tool, we can deploy it far more intelligently, more precisely and effectively with confidence we are looking for the right things.

OK, OK, I get it, so why do we do code reviews?

Looking at some of the popular search results for ‘code review’, we can see many posts providing us with a set of guidelines we should or could follow.

For example, in a post that lists ‘10 best practices for code review’, we see that the very first two points tell us how many lines of code we should be reviewing at once, and the rate at which we should be reading these lines of code.

The reasons for this, we are told, are because deviating from this will affect our defect detection rate.

Ah, ok, so code reviews are about detecting defects then?

There are other points, and indeed many other articles, that all focus on code review from the perspective of being an exercise in detecting defects in code before it goes live.

For many people, this is the sole reason for performing code review.

There are many other posts, that tell us there could be other reasons for doing code reviews.

The most common ones are:

  • To ensure that appropraite ‘coding standards’ are maintained
  • It’s a learning exercise for the reviewer, or a way of sharing knowledge
  • It helps to detect bad or wrong design
  • It encourages collaboration
  • It ensures non functional requirements such as security or performance have been met
  • It ensures the appropriate tests exist alongside the code

So how do we do code review?

So, with 90% of the developers and teams I have worked with, it goes something like this:

  1. A developer finishes work on something, raises a merge request, and moves the Jira card from ‘Development’ to ‘Review’
  2. The original developer will either assign the merge request or review to someone explicitly, or request that someone who feels like reviewing this should pick up this review task
  3. The reviewer will independently look at the code in the merge request, and make a number of comments against the changes in the merge request
  4. The original developer and reviewer go back and forward a few times, making changes to address comments, re-reviewing etc
  5. Once the reviewer is happy, they will either accept the merge request (and responsibility of it), or let the original developer know that they are happy for the request to be merged
  6. The Jira card moves along to the next step, typically some kind of ‘Ready to test’ or ‘Test’ state

This is how it tends to happen most of the time.

Sometimes developers will sit together to walk through the review.
Sometimes there are more formal review meetings.

But in my experience, this is how most people implement a code review process.

Does this process work?

It can do. I have seen it work, but often there are a number of issues with this kind of workflow.

One of the biggest problems, as mentioned before, is that people go into this with the wrong mindset. They perform code reviews, mainly as an unfocused feel-good exercise. Yes, good reviewers will catch issues, give good feedback etc. But this happens unreliably because most people don’t have a shared view as to what they are trying to achieve with these reviews.

Another big issue is that, especially when code reviews are the time that feedback on design is given is that often this feedback comes to late to do anything productive with it.

It’s pretty demotivating to find out after having written a lot of code that someone else thinks we should change it all. No one wants to be in this position, and to be honest most people wouldn’t want to have to tell someone this either. It’s far too late for code review to be an effective way to validate design. This leaves you with either two developers feeling uncomfortable, or sub-optimal design getting through.

Also, due to the asynchronous nature of a code review, often by the time feedback is given, it’s an interruption to the thing the developer is trying to focus on.

Note, I’m not saying don’t code review. Please please do code review, but do it intelligently, with the pitfalls in mind.

So, what should we do?

Treat code review more like a tool used to achieve something, rather than a process.

This means first of all, working out, and agreeing on the goals you have, the things you want to be able to verify and try to protect against.

So, you probably want to agree on design standards, coding standards, what your approach to testing is, what kind of non functional requirements you have.

Once you have agreed these things, written them down somewhere, then for each change, rather than trying to cover all of those bases at once, after the code has been written, think about how and when you can ensure those goals are met for any given change.

Decide which tool or tools you can deploy most effectively to verify the things you care about for any given change.

The key thing is, some changes may require more or less review, at different points in time. For some changes you may want to discuss and decide on the design before doing any code. Sometimes part way through coding design choices come up. Some changes are so simple it isn’t an issue.

Sometimes, the way to ensure coding standards could be through the appropriate use of static code analysis tools.

Sometimes it could be more effective to just pair program the change with someone who knows the area of code being changed well.

And if traditional pair programming isn’t right, then how about cooperative pair programming?

For knowledge transfer, maybe code review isn’t the best tool to use. If learning is a goal, then don’t expect someone to learn how something works and at the same time be capable of giving critical feedback.

If you approach this from place of goals, the needs you want to meet, not from process, and recognise code review is a tool, then it frees you to make intelligent choices on a case by case basis, with full knowledge of what your goals and standards are.

So, it’s not a step in a Jira driven process. It’s a way to make sure the standards you set for yourself (and agree on) are met.

Hopefully this should mean then when and where you do decide to have someone review code, you do it at the right time, and both parties know exactly what to expect, and can actively engage with the process, and see it as a good thing, providing benefit. Something more collaborative than moving a Jira card along and hoping someone will rubber stamp it.

References

Credit goes to the people and pages linked below. Check them out, but remember to work out what you are trying to achieve in your specific situation

https://github.com/thoughtbot/guides/tree/master/code-review

https://blog.fogcreek.com/increase-defect-detection-with-our-code-review-checklist-example/

https://msdn.microsoft.com/en-us/library/ms182019(v=vs.100).aspx

http://jibbering.com/faq/notes/review/

https://www.atlassian.com/agile/code-reviews

https://cwiki.apache.org/confluence/display/QUICKSTEP/Code+Review+Guidelines

https://mozillascience.github.io/codeReview/review.html

https://everydayrails.com/2017/01/16/code-review-mindset.html

https://blog.mavenhive.in/pair-programming-vs-code-reviews-79f0f1bf926

https://developers.slashdot.org/story/16/01/21/1749247/code-reviews-vs-pair-programming

https://arxiv.org/abs/1706.02062

https://collaboration.csc.ncsu.edu/laurie/Papers/XPSardinia.PDF

http://engineering.pivotal.io/post/pair-programming-in-a-distributed-team/

https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

http://www.evoketechnologies.com/blog/code-review-checklist-perform-effective-code-reviews/

Leave a Reply