Saturday, April 11, 2015

Reviewing the code review

Lots of software development based companies do code reviews, yet for the most part I find that code reviews more of a laugh than anything, well at least how it's done in the formal company setting. 

What I have observed is:
  • People who submit code reviews are just doing it because it's part of the process, that unnecessary evil that you have to endure to get your code merged into the repository ?
  • Unnecessary ? How many people who do software development daily really keeps an open mind when their code is sent for review and suggestions are made ? What I find is these developers already find their code working perfectly and does not need a change. If that is the case, why not just send me an email telling me that you will be checking so and so code into the repo and just give me a heads up. I know where you stand and you are not really asking me for a code review just letting me know that you will be checking stuff into the main repository. 

A code style document guideline

Code styles are like belly buttons .... any programmer who has written more than 10 lines of code using a language has one. Let's face it, everybody has an opinion on why 80 column rules matter or how stupid indentation rules are. Everybody has preferences and trying to cater for everybody's taste preferences is just stupid. Make it clear. This is the code style we prefer here. All major projects OS have it.

Tell me this, if we can have guidelines on code style, standards on testing and others, why don't we have a team agreed standards on reviews guidelines or more aptly called a code style document. Essentially what should be reviewed and what is considered superfluous. What is is considered good style and what is not. I for one would find it extremely useful if there was a guide that told me what is considered useful review and what is considered Nazism before I do a review instead of being beat down by a snide retort in a code review.

Each team too has their own "unwritten rule" about what is deemed important and what is not as part of code style and reviews. Why not make the unwritten rule a written one, giving an easier time the newbie or the outside joining the group. The bulk of the time should be spent feeling around the nuances of the code not for feeling around where the invisible lines and rules are.

You know what, if a standard is not going to be obeyed within a code base, it's really okay, just frigging document it down somewhere so that every single person who work on the code from day one knows it if they read the documentation. If they don't then beat them into pulp in the code review.  I would rather spend a few minutes bitching about how I feel a style is stupid but knowing not to bring it up in the review rather than spending time on a to and fro session in the review process trying to prove or defend how smart I am.

The goblin here does not lie in a bad style rather inconsistencies. Reading and understanding code, traversing the code logic is already hard enough, why add another layer of style inconsistencies to make matter worse ?

Make the unwritten erm ... written ?


If there are too many "unwritten rules" in your team, usually one of these things exist:
  1. A few senior coders or developers who have a certain idea of what constitutes good style which deviates from the standard recommendation but is unwilling to put into written rules.
  2. Learning the rules is thought of as learning process and part of the induction of a new member to the team. 
  3. There are a few opinionated team members who have unofficially beaten others to a pulp in a prior review, but still again did not document or went the step of formalizing what they think into a style document or rule. Refer to #1.
Both in my opinion results in waste of time. Why all the unwritten rules ? Just write it down, document it if the team feels that this is important enough to make into an "unwritten rule". #2 should not be used as a learning process. If this is part of the learning process, think of the countless hours and saved effort in to and fro of reviews you can achieve by just writing and documenting what the team or company considers as their internal code style practices.

Rules to enforcement


Collect all of the agreed style practices and using something like editorconfig, enforce it!  If a junior member feels strongly enough to change a standard practice, then use something along the lines of writing a Python PEP process to filter their conviction behind their reason for change. You want something changed, write a proposal on why the team should adopt it. Vote on it, deliberate over it but above all else document it! I know everybody wants to be a liberal, hear out everybody's idea but at the end of the day I think a lot of frustration and hassle could be avoided if everybody was clear right from the get go what is open for discussion and what is not.
Post a Comment