Have you ever been told….
…that your code sucks, that one of your ideas is horrible, totally unusable or even (alas) that you or other developers are dumb because they choose to do things in certain way? I have, once or twice…. :-).
Leaving aside the fact that the person proffering those kind of comments has probably not reached maturity yet and/or has not good people skills, there are some good tips to take into consideration when that happens.
I’ve stumbled upon this excellent article: http://mumak.net/stuff/your-code-sucks.html. I will simply copy-paste cites from it in blue. It talks specifically about code reviews, but we can adapt its tips if we think of a comment like “mocking/stubbing is a horrible idea and it can hurt the development process” as if it were a code review… (By the way, David show us in his article the proper way to handle these kind of statements. Excellent post!)
First, don’t take it personally. Very often programmers tend to confuse personal preference with objective worth. I tend to take the “let’s do it your way” path -in cases where there is no harm in doing so- to avoid never ending discussions.
In the positive side of things, consider:
…someone has just tried to improve your product. They’ve put thought, effort and creativity into helping you, and now they have put their work up for critique: thank them.
Divmod have a policy of always saying one good thing in each code review. There is always something nice to say, even if it’s just
I’m glad someone is looking at this part of the code
Code reviews provide an amazing opportunity to grow as a programmer and to improve the software we make. Off course! Just because the comment does not come wrapped in a polite sentence does not mean that it is wrong.
At the other side of the coin, If we are the ones reviewing other people work, then:
This ought to go without saying: review the code and not the coder. Comments about a person will only make it harder for that person to apply critiques about their code.
When making negative comments, refer to
the patch or
the branch rather than
you. For example,
You’ve introduced a bug in get_message becomes
this patch introduces a bug in get_message.
If all you say is,
this patch introduces a bug in get_message, then you have failed as a reviewer. The goal is to improve the code, not to provide a series of puzzles for the author.
The author should be able to look at a review and be able to tell how to address each point and also when they have addressed all points. Reviews with unclear outcomes turn into open-ended discussions about the patch, which sometimes become focused on making the reviewer happy, rather than improving the code.
Confusing personal preference with objective worth
This is a problem in all spheres of review. Film critics, literary editors and acadamic reviewers all do it.
What I like is not necessarily the same as
what’s good, although part of becoming a better programmer is having your preferences align better with reality.
What I dislike is perhaps even less likely to be the same as
When reviewing a perfectly acceptable patch that solves a problem using imperative-style programming, do not criticize it simply because it isn’t in a more functional style. Doing otherwise makes reviews a game of
guess what the reviewer likes rather than
write good code.
Reviewers can avoid this trap by phrasing review comments as questions,
Did you consider using a more functional style?,
Why aren’t you using regular expressions to solve this problem? etc.
I encourage you to read Jonathan Lange’s post, there are a lot of excellent tips on it!