We all know it and hopefully we all do it: Code Reviews! But have you ever thought on how your comments may be perceived? Or have you ever been on been receiving end of a frustrating comment on your code?
Disclaimer:
When I talk about code reviews, I mostly aim for code reviews done on pull or merge requests in a written form. But most of this probably also applicable to actual face-2-face/pair code reviews.
Code reviews are something very intimate for developers, as we share our work with others, in order to get feedback. Sometimes, feedback can be harsh, and this is maybe why people are sometimes afraid of opening their code for to review.
But wait, shouldn't there be a common way of communicating feedback? Even on things that didn't go so well or could be improved? Maybe in a constructive manner? Oh yes, there is a way!
Here are a just few bad examples of rather less constructive comments on a pull request:
- Why?
- This doesn't any make sense.
- I don't see the point of this.
- This SHOULD NOT be implemented like this!!!
- Why are you doing this?
- I don't like this.
This are just a few examples to highlight bad and not constructive comments. Depending on how those lines are being read and perceived, they are also destructing the confidence and working morale of the review seeking developer.
Furthermore, any of those comments are the perfect start of a long, long, long text-based argument. And instead of having a few constructive comments and improvements, you'll end up with an 80 comments long code review where everyone is defending their own view.
Is that helpful? Probably not!
Okay... But what if something is wrong with the code or you really don't understand what is going on? How about not writing comments, just approve and refactor it silently later? Mhm... This doesn't seem right either, ha?
Here are few tips on how to improve comments on code reviews:
Be constructive:
Instead of just highlighting an issue, offer alternatives or ask for a quick white board or on-screen discussion.
Write sentences:
The comment "Why?" is not really question. How about something like this: "I'm having trouble understanding this, maybe I'm missing something. Do you have moment to explain this to me?"
Don't write in uppercase:
DON'T WRITE IN UPPERCASE, AS IT IS USUALLY BEING READ AS BEING A BIT AGGRESSIVE!!!
Don't dictate:
Instead of writing "You be should doing it this way: [...]", how about: "I've a few thoughts around this one. Could we meet for a quick chat about this?". Or maybe: "Nice! I recently found this on www.dev.to/some_article. Maybe you could give this one a try? Let me know how it went."
Keep it short and sweet:
Writing an assay about something in code review is usually not a good idea. This usually leads to very unproductive text-based discussions and sometimes loses track of the actual issue. Create a ticket for the topic to talk about or set up a meeting. This way it will be more open for a wider discussion with the whole team.
Imagine it would be you:
Do you like to read bad and not constructive comments on your code? No. I thought so much. Talk to people the way you would like to be talked to.
It's nice to be nice:
Saying phrases like: "Good work", "Good step in the right direction" or "This a good improvement", don't hurt to write. And even if the code in review has some flaws, someone spend time to write it and had the guts to open it up for people to review. This should be celebrated with appreciation!
This list could probably be extended and improved on, but I guess the message is clear. I believe, if everyone respects and follows basic communication guidelines, it will lead much less friction within discussions and will enable a team to focus on the cool things, like: Making Awesome Software together!
Here a couple of questions:
Do you know any no-go-phrases you have seen in code reviews?
How do you deal with bad language within code reviews?
How do you deal with a change you don't agree with as a reviewer?
Top comments (7)
The worst I've seen is "Nope."
For me, code reviews with more junior developers usually turned into more of a training/mentoring session, which is perfectly fine and pays off in the long run. You just have to make time for it and not rush through it.
On the flip side, code reviews with more senior developers were really mostly dependent on their personality. I worked with a developer who was known to be very short and blunt. He would say things like "why you do this way?", which could seem off-putting at first, until you spend time with that person and just learn and know what to expect. I eventually learned how to best communicate with him over time.
Yeah, it always depends on the relationship between the devs. Like you would ask someone you don't really know "Would you like to go for a beer?", but with your best buddy it might be enough to just say: "Beer?" :D
In cross-site-teams this could be in issue. The team members get rarely the chance to get to know each other properly and some people might be put off by certain ways of communicating.
For me short and blunt is fine, but still would like have some sort of respect within a comment and at least the option of figuring out the why.
"Nope" feels kind of like = I don't like it, I don't see the point, but I also don't want to waste my time to explain you the why I feel like that.
Ah the classic one word code review comment:
etc.
It's hard to believe you could see comments like this in modern times, but you do.
I find myself sometimes in a "battle of arguments". Very subjective opinions, everything gets questioned for the sake of because one doesn't like it. And suddenly there is a pull request with 60+ comments. Also not really effective.
I remember one particular situation, when there was an argument about if the http-error-code should be displayed in the UI or not. That one had a total of 80 comments.
After getting some sort of feeling for those kind of situations, I've started to cut down those comments by saying: "If you have any further questions around this, I'm more than happy to describe the reasons behind this on a white or on-screen. Just let me know."
"Lame" is also a rather interesting one!
I really like this, as a junior developer myself, it's very off-putting to have someone criticize your work like that. And definitely makes you want to not work with open-source projects.
I think it's our job as developers to not only be good with code, but people as well, after all, there is usually a lot of teamwork involved.
Yeah, very true! 👍
I would even widen that to all sorts of projects where multiple people work on an not just open-source.
Criticisms it self is good, but is always about the HOW it is delivered. When talking to a junior you could say something like: "Good start! Maybe you could have a look to class-x/documentation-x/blog-x. Let me know if you have any questions." ...something a long these lines. Or just spend some time with your juniors and do pair programming :)
Love this. Be nice is always good ;)
When you see crazily bad code reviews(bad code style, bug, page-long function, ... etc) repeatedly from one person, what do you do about it?
In the base code review, I think perhaps we need to be constructively cruel to set up a high standard. Otherwise it’s not only painful for next few months for the team, but that person will get fired w/o being alarmed early enough.