How many people does it really take to ensure that code is correct? I’m faced with a bit of a paradox. On one hand, having ~20 people on a code review seems like overkill. It has the potential to slow down reviews and create a bottleneck to getting anything done. On the other hand, what if these 20 people want to know that the code changed in a certain area? And what if having a wider range of eyes on the code catches errors?
Code Collaborator has a nice feature called subscriptions. The user is allowed to subscribe to either a particular file or folder, or to a person. Then, when an author submits a code review which triggers the subscription, subscribers are automatically added to the review. The author has the option of leaving on subscribers, downgrading the subscription level, or removing subscribers completely before the review goes live.
There are two levels of subscriber. If one subscribes as a “reviewer” then their approval is needed before the review can be closed, but authors may downgrade reviewers to “observer”. Observers can comment and log defects, but their approval is not required for the review to be closed. If one subscribes as an “observer”, the author has the option to remove them from the review completely.
But why? Why would you want to remove an observer from a code review? I assume if someone subscribes to code, that they are either familiar with how the area works and would be able to make useful comments, or that they are working in that area and need to be aware when things change. As a scrum master, I also subscribe to my team members so I know when features hit review and so I can keep an eye on the progress. It seems like a no-brainer to me that subscribed observers should be left on.
Recently I encountered a review where the author removed all subscribed observers. When I asked why, it eventually came out that they found some observers to be annoying, and feared they would make useless comments that would delay the code getting through review. My initial reactions was that the author was being lazy and trying to get away with half baked code. After some discussion though, it came out that perhaps there is a serious lack of code review etiquette going on. Are people being unnecessarily pedantic? Is it actually more beneficial to remove these people?
Code Collaborator lets observers make comments, or if the offense is serious enough, they can log defects. Defects must be addressed before the review can be closed. But what does that mean? What types of offense are serious? I’ve seen people ask questions on reviews and log defects because they really really wanted the question to be answered before the review was closed. I’ve seen defect filed on coding style issues that a proper code beautifier could have taken care of. But then there are the more subtle points – what if a Mock type is changed to be less strict? What if there are two valid ways to do something and trade-offs for each? Is the code review the right place to be having these types of discussions?
In the end, my opinion remains that observers should be left on the reviews. They cared enough to subscribe, they want to know when it changes, and so I’ll put up with whatever they dish out. I’ll bet the code base will be better off for it. I prefer brutal reviews, it is one of the best ways to grow as a developer. Things that might be considered “picky”, like misspellings, I think greatly detract from the quality of the code. The code might not function any differently, but if we as developers don’t care enough to clean up simple stuff, then soon other concessions will start to be made. We need to have enough pride in our code to want it to be the best it can be, and the additional time and effort of having a few extra eyes on reviews is well worth it in my opinion.
