Software, technology, sysadmin war stories, and more. Feed
Monday, March 19, 2012

Code reviewers who rejected valid comments

Yesterday, I talked about comments and snarkiness in config files. Now I want to get into something a little more annoying, regarding comments in source files, or rather the lack thereof.

This isn't the standard rant about what comments are for. I'd hope that by now, people have learned that documenting the "why" is far more important than listing the "what". Saying "this function opens a TCP connection" is of variable utility, particularly if the function itself is named open_tcp or tcp_connect or something like that. A comment which explains "this is only here because the stock library only does blocking connects and we need nonblocking behavior" would be better.

But again, this is not about that. This is about what happens when you mix up comments, code reviews, and source control systems.

Again on the accursed LAMP project, I wrote a change which did something relatively magical to make a test start more quickly. It was the sort of thing which tried to copy a bunch of data from a central repository at startup, and usually ran a whole bunch of times on the same few hosts. I figured we had lots of disk space out there, so why not cache it locally between runs?

We weren't trying to measure the performance of loading that data onto the box, so removing the load from the setup phase made no difference. The test also did its own "warmup" scheme every time, so there was no concern about copying vs. not copying the data before it started measuring things. I just wanted to make it so we could get more runs in during a given span of time.

Also, given the relative unreliability of this foul testing project, I needed tests which would start as quickly as possible. If I had to wait 30 minutes for something to start just to do another iteration, it would take forever to get anywhere.

Anyway, I sent in my change with a comment which explained why I was writing data to this particular location and why it mattered. Now I had to get it reviewed, as per the norm for projects there. A couple of people on the project decided to stick their noses in and said the comment did not belong there.

When pressed, they said "the description of the change list would be sufficient". Yes, that's right. They actually wanted me to leave out a one or two line comment in the actual foo.py to keep it from "bloating". Anyone who wondered why it was like that could just go dig around in the archives of our source control system to find out what had happened.

I wish I was making this up. Instead of having the answer to someone's inevitable "WTF" right there in the code, they were expected to run "blame" on the file, figure out who had touched that section, and then look at all of the changes which might apply until they found the right one.

That, of course, would lead them right back to the discussion we were having right there, and the whole thing about not having the comment in the source. I think I wound up giving in to this stupidity and just let it go without the comment, just like they wanted. There were bigger battles to fight and I just wanted to get this checked in and move on.

I did add one little note to the proceedings, though. One of my messages on the review thread includes something like "Hi future people! Glad you visited this thread to figure out what's going on here" down at the bottom.

With that kind of thinking, I guess it's not surprising that the project was such a disaster.