This piece on code reviews landed in my email via an O’Reilly newsletter this morning.
I’ve posted a brief response to it but I wanted to discuss it a little further here. One of the core issues with some code reviews is that they focus on optics rather than depth. How does this code look?
There are some valid reasons for having cosmetic requirements in place. Variable names should be meaningful, but in this day and age, that doesn’t mean they also have to be limited to an arbitrary number of characters. If someone wants to be a twerp about it, they will find a way of being a twerp about it no matter what rules you put in place.
However, the core reason for code reviews should be in terms of understanding what a particular bit of code is doing and whether it does it in the safest way possible. If you’re hung up on the number of tab spaces, then perhaps, you’re going to miss aspects of this. If you wind up with code that looks wonderful on the outside but is a 20 carat mess on the inside, well…your code review isn’t understanding what code is doing and it’s not identifying whether it is safe or possible.
So what I would tend to recommend, where bureaucratically possible, is that before any code reviewing is done, coding standards are reviewed in terms of whether they are fit for purpose. Often, they are not.
It won’t matter how you review code if the framework for catching issues just isn’t there.