Criticising code

Question

What is the best way to point out flaws in a developers code? Stuff they've been working on a while and though you'd love to not say anything, it's bad code and something has to be said. How do you go about it without them getting angry at you?

Answer 1

Bake them a cake, and after they have a slice tell them, "By the way, your code sucks."

Seriously though, you just have to say it. If they get offended or hurt by it, then thats their problem. In order to get better they need to take the criticism. This kind of goes back to Jeff's post about bad apples: http://www.codinghorror.com/blog/archives/001154.html

Answer 2

The key is to talk to the code, not the person.

Make suggestions like "Our standard is to use a for loop instead of a while loop for iterators" rather than "You used a while loop, and that's wrong."

Another potential avenue is to ask questions like "Why did you use a while loop here?" that can lead to a discussion.

And when your code is the code being reviewed, remember that you are not your code, and that you shouldn't take things personally.

Answer 3

I have Aspergers and this sort of thing comes up a lot in my life.

Sometimes I just want to scream that's crap and you know it is and with some people I know I can and we can follow that verbal assault with fixing the problem and remain friends.

Generally that's not a good idea. Even with fellow geeks.

I tend to ask why they came up with the solution they did and lots of people grasp the problems as soon as they attempt to relate them to others. It's quite easy during this chat to point out you think portions could have been done differently and you think another option is better. If the conversation is give and take and not a lecture even the hardest nosed person will admit they missed the failings in their code and let go.

The worst people are those who get defensive about their code. Those are the angry ones ;)

Answer 4

Send them an email (only to them!), point out some of the bad places, and offer suggestions. Using references might help too, so that they don't think you're just pushing your own practices on them.

Answer 5

You should only do this for BAD code, not "huh, not the way I'd've done it".

"Hmm, I think that can be done a bit better."

Then show them.

Alternatively, wait until they're on vacation and change it :-)

Answer 6

Sit down with them and go over the code. Let them talk through their thoughts - maybe some stuff sucks because it has to. It's unavoidable not to hurt people's feelings, but most professionals should be able to accept constructive criticism. If you know of a programmer who's particularly sensitive you should (a) review the code more frequently (as opposed to 1 giant review in 2 months) and (b) make sure to do the review quietly and in private.

Best thing to do is to have a peer review process on checkin. Its always easier to accept criticism in small quantities from a peer versus mass amount of criticism from a lead.

Answer 7

Help people figure out the "better" way on their own. Then they'll consider themselves to be the smart one, and there will be no resentment of you. All of the best managers I've worked for were excellent at helping people optimize on their own without pushing some mandate. It really helps relieve the possibility of resentment.

Answer 8

This, to me, comes down to two things: your ability to criticize constructively and tactfully and the coder's ability to accept your criticism.

In order to criticize effectively, you need to be able to point out specific flaws in the code, rather than just calling it "bad". Be able to explain these flaws and offer examples for why you feel the code is bad. You also need to be able to offer suggestions for how to improve this and future code. If you can't do these things, then it's going to be hard to convince this person that their code needs to be changed.

Be generally tactful in your language. (For example, you probably shouldn't call the code outright "bad" to the person's face. He/She will probably resent that.) Be careful to make sure that you confine your criticism to the code rather than the coder. That's the main thing you need to emphasize: you're not attacking the person; on the contrary, you're trying to help him/her by improving their ability to write code and improving your organization's code base.

The problem is that some people just can't accept criticism very well. There's not much you can do about this. Criticism is a big part of coding with others and you're not doing anything wrong by criticizing as long as you do it the right way. I think that if you follow my advice above, you're fulfilling your part of the obligation.

Answer 9

I agree with Karl Seguin. The big mistake is to wait too long to review the code of one of your programmer. When I see someone is making big mistakes I do several things:

• reduce a bit the amount of things he as to get done (not always easy)
• review the code 1 time every day with the programmer
• review the code with the programmer and another randomly choosen one (that's the best part I think)
• organize programmer talks about a specific subject (naming variables, class diagram etc...) and do some exercises/labs.