Why Code Review Sucks
I hate code review. Correction, what I hate being responsible for the final code review. Having my code reviewed by someone else is fun and educational. Doing a first level code review is great! I love helping others. But being the gatekeeper? That is no fun.
There are three phases to final code review. Two of these are also part of the first level review: code style review and suggesting improvements. But final code review includes saying "No" to deploying bad code. Saying "No" again if it is still bad. Saying "No" again when it was changed properly, but new bad code was added. Saying "No" as long as it takes to fix bad code.
Code Style review really should not even be necessary. Most of it can be automated and done by a computer. PHPStorm builds it right into the editor. What is missed by automation is caught by the first level review. By the time second level review occurs there should not be any issues. Every now and then something slips through. If your lazy you can skip it. I am not lazy, so I do not skip it.
Improvements are fun! It is a chance to share and discuss different coding techniques. The initial developer has a chance to make his code better. Both parties get a chance to learn something. Often I will run across a bit of code that does not "smell" right - but nevertheless within the framework the code runs it is the correct solution. Since improvements are, by definition, not vitally needed - they do not block a release. If deployment is urgently needed, simply add a few //FIXME and //TODO comments and release.
Rejecting bad code is no fun for me. Repeatedly rejecting bad code tends to reflect on the the gatekeeper - not the programmers. In my experience, a good gatekeeper is one of the first people to be fired after six to twelve months. Management can become very frustrated with delays and cost overruns.
The gatekeeper is responsible for any problems due to code approved. If something breaks, you are responsible for staying as long as it takes to fix it. One, Two, Three in the morning. It is your ass on the line if over the course of 6 months, the company racks up 40 to 60 hours fixing problems caused by the code you passed.
I could avoid being perceived as an obstructionist by passing suspect code. Given the choice between :
- Being fired for adding 60 hours of time to fix problems after they occur, or
- Being fired for adding 60 hours of time to avoid problems to begin with
The choice is easy, go with option 2. It is the job of the project manager to overrule a veto and justify it. They are responsible for risk assurance, not the gatekeeper. If I am fired, it is far better to be able to honestly answer the 'leaving previous position' question with: I refused to approve the release of broken code.
What is the definition of bad code. There is no set definition. Every good gatekeeper makes that call based on experience. "Experience" is a nice way of saying "stupid mistakes I had to fix in the past" . Really bad code is "and it took me 20 hours to fix!" And I must stress, by the time bad code is still working code. Unless the developer and first level reviewer completely half assed their job, the code has been tested. It does precisely what was intended under a specific set of circumstances.
Bad code is code which is written in such a way that it might cause an issue. Under certain circumstances. For a specific 'edge case'. Generally the developer is convinced that the issue will never arise. The project manager and developer will argue that time is being wasted. My rough estimate is that for 80% of rejections due to bad code, releasing it anyway would not result in any tangible issues. Out of every ten hours, only two provide a tangible benefit.
The benefit of writing good code ranges from 4 to 40 hours of support time that is saved. For larger issues which take over 16 hours to fix, of those 16 hours at least 8 of them will be spent by the final code reviewer! Fixing a stupid mistake you knew could cause a problem to begin with. That gives you a very large personal incentive not to approve bad code.
Time saved is a metric most companies have difficulty tracking. How do you know precisely which preventive method saved time? And how do you measure how much time was NOT spent fixing a major site down issue? I can measure that time based on past experience, but I cannot prove it to someone else.
So I hate doing final code review. But when it is assigned to me, I refuse to do a half assed job. I make sure to communicate that I do not this position permanently. It is stressful and in my opinion should mandate hazard pay. Heck, if the hazard pay is large enough I might even be willing to do the job permanently.
Nevertheless, I often find myself taking on this role on an interim basis. For example, my company had an extremely competent and excellent gatekeeper. When he decided to move on, I stepped up on an interim basis. I am now the gatekeeper. So I just keep reminding myself of an underlying philosophy required for good developers and good employees: 'Suck it up'. :-)