Wednesday, 24 August 2016

Code Reviews 2 - Dysfunctional Code Reviews

We've all had them; code reviews that just aren't effective. Here are a few scenarios.

I would love to hear your feedback about other types of reviews you have encountered.

As a submitter:

The '0' feedback code review

You eagerly prepare your code, a few methods and a few new tests. Submit it to the guild. Appoint the master code reviewers. The reviewers give it a +2 a minute later, with no feedback what so ever. WFT... surely there has to be something wrong with it?

The "Trivial" details code review

You submit your code to the review board. After a wait, the results come in. The reviewer wants the variables and methods in alphabetic order. They insist on putting in all the full stops on the comments. They are making it a blocker that you don't know the difference between "it's" and "its". They point out you are missing capitalisation in your javadocs. You haven't expanded three letter acronyms on their first use…

The "goes on forever" code review

We submit out code for review. A few days later, the first set of reviewers find numerous "serious" issues. We address all the comments. It goes out for review again. The following week, the next set of reviewers find another set of serious issues. We address all those and our code goes out again. Another week goes by and next set of comments back highlight another, different set of "serious" issues. We address those and the code goes out to the reviewers again. The next set of comments back and highlighting some of the same stuff as the first review, that the third code review made you put in…

The "Self" review

You are using a code review tool because the organisation process requires you to use one. However your team has no real interest in each others work. When you make changes, you put them up for review. But you always push through your own code anyway.

As a reviewer:

The "token" code review

It's 16:40 on Friday. The sprint ends in 15 mins. Bam! Into your mail box, comes a review for a critical feature for this sprint. The change set is long, several classes with a few hundred lines of code. Just remember it has to be in the main branch by 16:45… and that includes any re-work! It only takes a fraction of a second to hit that +1 button and the weekend is yours!

The guy who is always right review

The author is never wrong. How can you criticise this guy? Their code is flawless. Always. Even when you spot a real issue, they won't listen to you. They'll justify the unnecessary, out of scope code in the solution with fancy subjective words like "extensible" and "future-proofing". Some sections of the code might be unreadable to you, but that’s only because you are a lesser programmer and we'll save several nano-seconds during execution because he used an obscure construct. Basically this code is right. You are wrong.

The guy who is always wrong review

This guy just doesn't get it. The solution proposed isn't just a little bit wrong, it's completely wrong. There are no tests whatsoever. There is no concept of the agreed style guide, he just copied a large chunk of code from anther part of the system - changed a few strings and it's the proposed solution?

The death by a 1000 updates' review

So you get the call for a code review into your inbox. You gallantly volunteer to take it on and you fire up the tool. You spend 40 minutes reviewing the problem statement, then the tests and pick your way through the solution adding your comments as you go. Next thing you check the tool and there is an update to the commit. Many of your comments are now null and void due to the changes submitted. No panic. You start picking your way through the new solution again. Thirty minutes later, in pops another set of significant changes, rendering many of your comments null and void... again! So you start the review... again!

The "rutting" code review

A senior developer has submitted their code for review. Another senior developer has taken up the challenge. These two never see eye to eye. The comments come thick and fast with slightly confrontational and inappropriate language. It's a thinly veiled "you are an idiot"! The gauntlet has been thrown down. The submitter is not going to take that sort of "threat" lying down and they respond in kind, in the tool. The insults and accusations go back and forth for all to see in plain text. The tension rises. Other reviewers fall away, afraid of getting caught in the cross fire. Sooner or later either one declares the other one is wrong or Godwins' law will apply! At this point someone else will step in, merge the code and promises to keep these two apart in future!

Updated 23rd September, 2016 Updated 3rd July, 2017

Tuesday, 23 August 2016

Cycle time and Delivery frequency

Delivery Frequency

Delivery Frequency is the count of the number of times we ship the product out of the team to a receiving organisation. That could be a test group or a "friendly" customer or another interested stakeholder.

Generally we favour higher delivery frequencies. The optimum number depends on our delivery infrastructure. This can vary greatly across products. But at a guideline, we are looking to ship more than once per day. On products with long build and test cycles, this can be a challenge. Why this is important:

  1. It's easier to integrate smaller changes, more frequently.
  2. We get earlier regression feedback on our changes.
  3. It can help reduce "traffic jams" at the end of sprint.

Cycle Time

Cycle time is defined as the length of time a story is actually being worked on. The clock starts once the story leaves status "open" and it stops once the story is marked "closed". Usually this is when the story is delivered out of the team. Why lower cycle time is important:

  1. We get an earlier demo to the product owner.
  2. Better chance of delivering something in the sprint.
  3. There is the possibility of more agility from the team. We can change new work planned before the stories are started.
  4. Small stories are easier to implement, are easier to integrate into the system and they are easier to regression test.

Relationship of Cycle time to Delivery Frequency

There is a coupling of Cycle Time to Delivery Frequency that is interesting, and observing the trends in both can be indicators on how the team is performing.

Cycle TimeDelivery FrequencyWhat it means
Low or trending downHigh or trending upTeam is heading towards high performing
Low or trending downLow or trending downTeam not delivering despite closing stories. Need to investigate why?
High or trending upHigh or trending upTeam is frequently delivering "undone" work - need to investigate why? Are stories correctly split?
High or trending upLow or trending downTeam might be struggling. We need to engage the team to discover the difficulties and challenges they are facing.

Tuesday, 9 August 2016

Code Reviews 1 - Why?

I'd like this to be part of a series of articles on code reviews. Comments and suggestions welcome. There are as many views and opinions about code reviews, as there are developers. These are my views. I hope they make logical sense. They have been formed through my own experience.

Why do we do code reviews?

There are primarily two reasons why we engage in code reviews. Both are equally important.

Code Quality Checkpoint

For any "product", a review should be part of the production process - this goes from a set of accounts to a presentation. Reviewing one's work is a great practice for any craftsman to part-take in. There are two quality aspects that code reviews can help with.

Code Quality

How can I know that the software I have written is "good"?

Code is written once. Code is read many times. Frequent code reviews help me to write great code.

Code reviews help to have a good code base in the product. A "good" code base is one that is readable by a majority of programmers, without too much intervention by the Author. This has been expressed as the number of WTF per minute. We can ensure any style standards are consistently met.

Product/Component Quality

Code reviews are the also important for finding any possible bugs or errors in the solution we have put together. Since code reviews occur early in the production lifecycle, they are one of the cheapest ways of detecting and preventing issues.

Knowledge Transfer

Code reviews are a medium of communication between developers.

When we start out, we share our work with our more experienced peers so that we can learn. As we get wiser, we share our work with our peers to get their validation that we are doing the right thing. When we are at the top of our game, so to speak, with many years of experience we also take an active part in reviews to help pass on our experience and to get exposure to possible newer ways to do things that more recently trained developers will have learned.