Confessions of a programmer: I hate code review (2010)

Rant...

Was nominal lead on a smallish project a couple months back, with 2 other folks. Worked with one before, one I hadn't worked with before.

We're all remote, and the 'new' guy started doing 'code review' on code before he had a running environment. We'd taken over another codebase, and he was pretty up-front about wanting to adopt/enforce certain stylistic standards, which... to me, I don't particularly care about as much (on the PHP side of things, PSR-2, in this case).

We're all remote, so we had a call - a couple actually - and I indicated I didn't care all that much, but he was free to make changes as he saw fit, as long as 1) he got a working environment up and running, 2) he was also spending time writing some tests around the code as he explored it, to have a firmer understanding of what was going on, and 3) he didn't break any existing tests (or new ones that were developed).

I swear that I don't think he'd actually had a working copy of the project, but was still committing code. Not a lot, but ... it broke tests. It broke stuff that was not 'tested' in an automated way yet, but was testable by running the app, and I think he'd just not bothered to test it at all.

But... hey - look at the formatting! Look - spaces, not tabs! - how useful when someone else is having to go back and debug your crap. I received a couple small lectures on variable naming and the importance of descriptive variable names.

function getListOfUsersForCompany() { $res = db::query('whatever query'); return $res; }

Apparently, that $res is confusing and 'bad practice' because it's "hard to reason" about what's going on there. Literally 2 line methods with an internal placeholder variable (which also had some working functional tests around them) - those were being criticized by someone who had committed test-breaking code multiple times.

In all this, I kept having to figure out how much of my reaction was because my code was being criticized, vs what the criticisms were. I got accused of taking things 'personally', but I kept coming back to 'working tests broke with these changes', and in my mind, that this code was considered somehow 'better' because it more closely followed a stylistic 'standard' was bothersome. Working tests should a standard we strive for too, right?

I have worked with some folks who had a real eye for analysis - remote or colo pairing on problems has helped reason out issues that I know I've missed. And when I'm pairing or doing a requested review, cosmetic style tends to be the last thing I'll bring up, because it generally has the least impact on something. Much prefer to have someone spend time writing tests and/or docs and/or insightful comments vs reformatting. When I've refactored with someone to make something more testable, better naming/comments/docs tends to flow out of that refactoring anyway.

While I understand formatting is often not that big of burden, especially with modern IDEs, it's also not terribly high value on many projects. I'm primarily speaking on projects I've come in to where there's already extant code - in this case, we had taken on a very weird hybrid of 2 PHP frameworks and 2 JS frameworks (some screens used raw vue, some compiled vue, and some compiled angular).

Nelson is brilliant!

much like 'testing', the code will get reviewed at some point - either during a crisis, or before it goes out.

The problem with code review is that it works. That's why we can't shake the industry's fascination with it. It works to catch bugs, but it's horrendously inefficient in doing so.

I ran some numbers on our code review process on a project last year. Of patches returned for modification, 5% contained a bug. 95% were for entirely stylistic changes. I find it hard to square that with being a good use of time.

As an industry, we're making the mistake Deming points out in Out Of The Crisis: we're attempting to inspect defects out of existence rather than improve our production process so that they never get created in the first place. We concentrate our efforts on being better at 100% inspection, and don't allow ourselves to think that a process without that inspection could be a hell of a lot better, if we invested the effort in figuring out what that might look like.

Pairing might be one answer here, although I don't think I've heard it talked about in those terms before.

Doesn’t sound to me like you are the problem. Sounds like your colleague has lost sight of the end goal and needs to refocus on what you’re supposed to be delivering.

(Unless the whole project was to reformat the code?)