…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:
Ad hominem
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
.
Unclear Outcomes
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 what’s bad
.
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!
virginiablanc 1:59 pm on Abril 2, 2009 Permalink |
Seguro que va a salir todo muy bien!
Felicitaciones por toda esta movida para tener una Rails Conf Internacional en Argentina: todo un logro!!
Franco 6:25 pm on Abril 2, 2009 Permalink |
Éxitos con toda la organización del evento, como dijo mi amigo Marley “everything gonna be alright”