Be excellent to each other
August 19th, 2019 - 3 minute read
My previous job didn't have a formal process for code reviews (not like most companies). We'd get people to take a look at something if it seemed a little off, but that's as far as it would go.
Now I'm in the swing of it, something that is really important to code reviews is politeness. It doesn't matter how correct you feel you are, if you don't word it politely you can't expect people to respect you or take on board what you are saying.
A couple ways to be more like Bill.
1. Check yourself
Instead of saying "This is terrible" or "Why didn't you just do this?" or even worse just straight up calling out a bad practice and saying "Do it this way" you can reword all of these to be much nicer.
"This is terrible" could be "I think this might not be the most elegant way, have you thought about x?"
"Do it this way." should be reworded and backed up with a solid reason. For example, if you are only supposed to use hooks in your application because that is the standard in the company and someone uses a class based component:
"Use a hook based component". - Helpful, but not particuarly empathetic.
"Here at x company, we have a standard where we try to use hook based components. Could you change this to be hook based or give a reason why we shouldn't?" - Helpful, empathetic and productive.
Going into a code review assuming you are much better than someone else is harmful for the culture and the other persons self esteem.
2. Negligble gains
This is a hard one, especially when you have a personal preference for doing something. If you have a particular style to your code, and it's not defined by your linter. Don't flag it up. Or better yet, have a discussion with your team and decide whether it should be added to the linter.
Another example of nitpicking is making someone change code even though functionality it might be the same. Perhaps your way of doing things is neglibly faster, but if you're not working in a codebase where you need to scrape every milisecond of performance out of it then leave it. Maybe have a calm discussion with the other person about why it might be worth changing it . It's not a reason to reject a PR, especially if they are fairly new to the codebase.
I'm not saying hold new staff to a lesser standard, but definitely be aware that they might not know everything about the style, or performance benefits of your special technique.
This goes for renaming variables, if it makes enough sense to be understood just let it slide. It's honeslty not worth either of your time.
Those two techniques honestly go a long way but I think the essence of a good review boils down to empathy. Be empathetic, and party on dudes!