Peer Code Review – Are they worth the hassel?

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 weel 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.

Advertisement

What’s a Story Point

What is it?

Story Point is an arbitrary unit of measurement to measure amount of work in a Sprint. It’s a pretty abstract concept and can be very hard for people to comprehend. Is it same as hours? No. Similar then? May be.

What is it then? It is something that you can relate to Sprint to Sprint. If your team accomplished x story points in one Sprint, given the same amount of resources and time, team should be able to accomplish more or less the same next Sprints. Once you do it a couple of times, you should be able to get your team Velocity.

Other Industries

If you take a look into Construction or Manufacturing industry, the measurements are pretty accurate and pretty standard. Everybody know what an 1 mm drill is (if there such a thing :)). You go to Home Depot, every part has measurable characteristics:  height, weight, width, color, etc. Why shouldn’t your Scrum Team be: Resources, Duration, Sprint Goal, Story Points, and Velocity.

Story Points are usually based a mathematical number series called as Fibonacci Sequence.

Fibonacci Sequence

Wikipedia has a good read on what they are. It essentially are numbers as 1, 2, 3, 5, 8, 13, 20. Note number on right is addition of last two numbers on left 8 = 3 + 5, 13 = 5 + 8.

Each PBI (Product Backlog Item) is supposed to have Story Points associated. This is done by a voting process (sounds funny, but it’s important). Only Pigs (Team members responsible for building the feature Dev, QA, UI/UX, Doc) are allowed to vote.

Voting is also referred as Planning Poker. Why Poker: Because Pigs don’t know what other Pigs have selected unless “Show” is called by.

Voting (Planning Poker)

During the Sprint Planning meet, the Product Owner spends about a minute explaining the PBI  that needs to be have Story Points assigned. In the next minute or two Pigs  ask questions to get clarity. Note: This is not a feature discussion meeting. Pigs and Product Owner should have a good idea about the PBIs prior to the meet. What you are trying to do is assign Story Points, so you can predict future Velocity.

Each Pig is handed over a deck of cards. There are seven cards in the deck with each containing one of these numbers: 1, 2, 3, 5, 8, 13 and 20. When asked to vote, each Pig should individually select a card and put it on the table face down. When asked to show, everyone should show their cards together. People on the phone are asked to tell first so they don’t get influenced by the outcome of physical cards.

Story Point Guidelines

This is the most important aspect of allocating Story Points. Every team member must know how to pick one of those Fibonacci numbers. It might be a bit easier if you think from an elimination point of view.

20 – Too big for the Sprint. You need to break the PBI into smaller PBIs. Ideally nobody should have selected this number in voting. If they have then be prepared to talk about it.

13 – Something that will take full Sprint to accomplish. You should also be very clear what exactly does done mean for the PBI. Is it just Code Complete, or does it include QA and Documentation. Ideally PBIs should be created in such a way that the Product becomes releasable at the end of Sprint. However, this may not be very practical. I’ll table that discussions for a future Blog. For now keep in mind that you need to assign 13 if you think this will be going on through out during the Sprint by 1 person. In a 15 day Sprint: 8 days for Dev, 4 days for QA, 1 day for Doc.

8 – Something that takes half the Sprint.

5 – 5 days (in a 3 week Sprint)

3 – 3 days (in a 3 week Sprint)

2 – 2 days

1 – 1 day or less

less than 0 – Combine such PBIs into 1 or more

Again, your definition may differ depending on duration of the Sprint. The above is based on a 3 week Sprint. With lots of experimentation we’ve concluded that 3 weeks Sprint is the best. Perhaps another blog to talk about that.

What you really need is a framework to which your team members can associate these numbers to. A 5 may be 3 days in your company.

During the first few Sprint your team members may be asking questions as: What does 3 mean again? and 5..hmmm.. how about 13. I don’t have card for 10? That’s perfectly normal to ask.

Re-Voting

It’s unlikely that everyone will come to the same number on the very first voting. It may be possible that majority come with same numbers with a few on the extremes.

People voted on the extremes should be asked to explain their rationale. After listening to arguments and arguments team should vote again. Move on to the next PBI as soon as you hit a consensus. Again it’s not necessary that everyone has the same number. In the interest of keep moving in the meeting you may pick the number with majority of the votes and continue on to next PBI. With time you’ll get better with all this.

IMO you should not go beyond two rounds of voting (including the first round) for a PBI. If you need multiple rounds then there is a bigger problem. People should have a good understanding of the PBI prior to coming to the Sprint Planning meeting. Focus on solving that problem first.

Total Story Points

At the end of the Sprint Planning meet you should total all the Story Points of individual PBIs and find out what the total is. This total is very important. Total will dictate your Velocity. If you did x this Sprint, you should be able x next Sprint and so on.

Still not convinced

Well, you are not alone, I struggled for a long time. All I can say is keep practicing and you will get it.

Asked to Work or Just Work

Have you ever wondered if you are asked to work or you just work. This is with reference to a workplace. There is a subtle difference between the two.

Asked to Work

Is there a Manager constantly trying to figure out what you need to be working on. It may also be that you are asking people around what you should be working on. If the answer is yes, then you are being Asked to Work. It means the managers running your company  hasn’t communicated the plan or worse don’t have a plan. IMO it’s recipe for disaster.

In a good company employees should have a pretty good idea what they are going to accomplish in next few days and weeks. Beyond few weeks is too much to ask; bare minimum you should know what you need to accomplish in a week.

Just Work

This is just the opposite. Work is so well defined that you simply Work. There is no one pressuring you to work. In fact you Love to work.  You just know the right questions to ask your team members / manager and simply work. You exactly know how to extract your piece of work from Work-Jar and simply rock and roll. I call that Work Nirvana 🙂

Let me know if you are Asked to Work or you Just Work.