How not to give helpful criticism

One of the things that people often struggle with it taking criticism.

Professional criticism though is part of the job. In software development we have code reviews, and the person doing it the reviewing is supposed to point out things that are either wrong, or could be improved upon. It is then up to the author of the code being reviewed to take this information as it should be: as constructive criticism where constructive is the operative word.

It has been pointed out before that programmers are insecure folk who don't like their work to be criticised, but the fact is that programming is a complicated task; one that is practically impossible for anyone to get right all the time every time with no mistakes.

This is what reviews are for, after all.

It is true though that people struggle with this: I have performed many reviews myself, and it is interesting to see the differing reactions from person to person. Some just get on with fixing what I point out, while others get very defensive about it. Of course, the reviewer isn't always right either, and sometimes you'll get a bit of debate between the reviewer and author on a particular point. This is all healthy, and the end result is better code, which is better for everyone.

Now, when I do code reviews I try very hard to be respectful of the person I am reviewing and of his work. I don't like to be offensive, or make the author feel like an idiot: as I say, people will always make mistakes, and may not know as much as I do about the particular item in question that I am pointing out. Again, this is to be expected: nobody knows everything.

But what if the criticism, while perfectly valid, is presented in an offensive way? How should one respond to that?

Recently I took over maintainership of a small bazaar plugin called diffstat. It is written in python, a language which I like but have little experience in. I figured this would be an excellent opportunity for me to learn more about python, and get involved in the open source community in some way in one go.

Today I was notified of a new bug against the plugin which I have to say got my back up a bit.

The report was not really a bug since the functionality was not broken. Rather it was pointing out how one part of the code could be improved. The diff is attached to the bug report, but in essence it replaces nine lines of code with two that work exactly the same.

It is a perfectly valid and good improvement that makes use of a feature of the python libraries that I was not previously aware of.

The subject: "Silly code for popping kwargs".
The message: "The code in "cmd_diff.run()" for popping the "stat" and "stat_diff" parameters is convoluted and silly. The optional default parameter to "dict.pop()" is a better solution."

Erm, what was that? Silly???

I immediately felt offended and even unsulted, hit the reply button and, hands poised over the keyboard, thought for a moment.

The criticism was valid: the existing code was, while not 'silly', definitely convoluted in light of the better way of doing it that the reporter provided. Furthermore, despite the poor way in which this was communicated in the bug report, this was a good improvement to the code that the reporter was providing in his own time. The bug report was in effect a code review, and I should deal with it as such.

So I thought about toning down my response a bit: while the review was valid, the approach was not, so perhaps making it civil while writing "silly" in quotes to make the point passively would do.

Another moment of thought put rest to that. It was childish, and furthermore anything I write will become part of the project's permanent record in the form of the commit log and bug comments for all to see for ever more.

So in the end I sucked it up, thanked the reporter for pointing out the improvement, and made the change to the code he suggested. I did throw in there that I was quite new to python, to attemt to explain away why the code was like that, but that's as far as it went.

I have no interest in making myself look like an ass online. The things is though, others might have no problem with it. Those people would have reacted differently to how I did in the end, resulting in the bug reporter taking his attention elsewhere to the detriment of the project. Of course, this could all be avoided by people giving their criticism in a better way in the first place...

Before reacting to anything on the Internet it is worth thinking for a minute about the consequences of doing so. The bug reporter reacted to the code he saw by calling it silly. I chose to react to his reaction by thanking him.

Some people are just rude, some don't speak your native language and might not mean to be rude. Others will go out of their way to be polite. Whatever, if they are trying to help, let them.

Don't let pride be a barrier to collaboration.