This blog has been cross-posted from my personal blog
Over the last many months the Engineering Organization at DotNetNuke Corp. has been making many process changes to deliver high-quality Software. As Scrum Master and Lead Developer, I’d like to dedicate a blog series on what we did, how it helped and the lessons we learned.
In this blog, I’d like to talk specifically about Peer Code Review that we implemented over 10 months ago.
Why need Peer Code Review:
Before I go any further, I must admit that the DotNetNuke Corp. has one of the finest Engineering team I’ve ever worked with; team is not only technically sound (few are actually Microsoft MVPs), but also has great work ethics, passion for what they do and above all great team work.
Might you ask if you’ve got such great people, why need Peer Code Review. Good question, here is why:
As you all know DotNetNuke Framework has been around for many years and is regularly under works for delivering yet exceptional product. At times Engineers end up checking-in code that results in unintentional breakage elsewhere. Some of the common breakage are build, package, unit test, automated test and software functionality. Simply put, despite their best efforts, it’s just not possible for everyone to know everything about the framework.
There must be a better approach to move aggressively but also keep higher code quality.
What’s the solution – Implement Peer Code Review:
As a team we analyzed the problem and came up with Peer Code Review as an inexpensive way to improve our code quality. After all the wise men have once said, one and one combined is eleven 
Why not use the power of two. We made a rule that every check-in must be code-reviewed by another Engineer.
Note the emphasis on word “Peer”, it’s not Manager, Lead or anything else, just a peer, which is another Engineer in the team.
In the DotNetNuke engineering organization, we have a concept of buddy-system, which is actually very helpful. More on this on another blog.
How does it work (logistics)
When an Engineer makes code change and checks-in code in TFS, he/she sets the Code Reviewer field TBD followed by Engineer name and notifies to that person by email.
Reviewing Engineer analyzes the code and puts their name in TFS changeset (replacing the word TBD) to indicate code review is done. If a problem is found the two Engineers work together to resolve and check-in more code as needed. The subsequent checks-in also needs to be reviewed. It’s an iterative process. Given, we’ve been doing this for many months there is usually less rework.
Initial Team Reaction
Most of the Engineers liked it on day-one as they can sleep well in night knowing their code is being reviewed by another peer. A few were hesitant as now every check-in must get approved and was a bit hard to digest. However, after a few weeks everyone bought-into it and made it a habit. I personally feel insecure if there is no code-review on my own code, call it self-code-phobia
Enforcement
We didn’t really have to do a whole lot to enforce this new rule; every few weeks we’d run a report (custom program) in TFS to see which changesets didn’t have a reviewer. Their names were called-out and asked to complete the code-review. This was done just a handful of times and after that it got into auto-pilot mode. However, as Scrum Master and Lead Developer I keep reminding people to keep doing code-reviews as it’s very easy to not do as well.
Not everything needs Code Review
Trivial changes such as adding comments, fixing broken tests or typo-correction doesn’t require code review. It’s optional, some do some don’t.
Is it a burden on time?
Definitely not. We don’t spend a whole lot time doing code review, but at times it can take over 30 minutes or an hour (or even two) a day, especially if the reviewer has been procrastinating and has a pile of reviews to be done Many times you’d here on daily standups as one of the main activities in last 24 hours. If you are on top of this, it’s 10-15 minutes a day.
When does it work
This is the most important part of this blog. It works when the person doing code-review is well versed with the code being asked to review. For e.g. two Engineer working on same area. On top of that if the other Engineer is waiting for your changes to do his stuff is also super efficient. That’s where I feel it works the most, when the other person not only does code review but also executes your code while testing their own stuff. We insist Engineers to work in teams of two; definitely on complex projects; however, it’s not always possible.
When it doesn’t work
Something is wrong when you are hunting for a reviewer:) Ideally there should be a natural choice to select the reviewer. We’ve run into cases where the person making code changes is the only person working in that area. In such situations review-task is assigned to anyone on the team; the reviewer is expected to do their best and call out issues. In such cases you need to insist Quality Engineers to be extra diligent in their QA process.
Another situation where code review is not that effective is when Reviewer is too busy with other work. Reviewer might rush through the review doing superficial scrutiny only. One needs to be careful while assigning Reviewer.
Review quality can sometimes get impacted when Reviewer and Reviewee are in different timezones. We haven’t had many problems due to this reason, however, it’s possible.
Is Code Review sufficient for Quality Code
Absolutely Not. This is a mandatory step that every team should incorporate in their Software delivery process. Great Software requires lots of other things (process, people, tool, etc.). More on other blogs.
Summary
Peer code review is essential to minimize bugs and maintain high-quality software. It works best when Reviewer and Reviewee are working on the same area or are well versed with the code.
Please feel free to share your thoughts as comments in this blog.