You’ve finished your piece of work, you sit back, let out a deep breath and your shoulders relax. You did it! It was hard, there was a time where you had to manoeuvre around a tricky problem, and there’s a small piece of code that could be better but it was the best you could come with up with. You’re looking forward to getting your teams feedback and hoping they may have a more elegant solution or have questions you can share ideas on!
But what happens if the feedback you receive makes you angry? Or upsets you? How do you deal with that? What have they said/done that’s provoked that reaction?
Or perhaps you’re the reviewer and your colleague has reacted in a way that you weren’t expecting. How do you feel? Do you feel like they’ve overreacted? Or worse, did they *deserve* it?
Rule #1 Ask questions rather than make statements
Be kind! Sounds obvious right? But it can be all too easy to get more concerned with the code and forget about the author and their intentions. How many times have you looked at a piece of code and thought “What the hell were they doing!?!?!?”. Take a step back and remember those were decisions that were made… on purpose. Try asking why the author chose a certain method over another, and not in an accusery way either. Come at it from the angle of trying to learn about the code, and from that make your suggestion in how it could be better. If your suggestion isn’t necessary, or more of a style choice you think should be favoured, or a more efficient implementation, then do just that – suggest it. No one likes smug people and it can embarrass the author as these reviews are usually public. Be humble and help.
Rule #2 Limit personal preference
As touched upon above, personal preference is your personal preference. Unless agreed upon beforehand, picking up on issues such as formatting or layout can distract from real bugs and errors in the code. I realise that the control-freak inside most of us gets frustrated at what we feel are silly design choices, but one single person alone is not in control of the code base. If two people have different views about these issues, there is no winning in an argument – so don’t waste your time.
Rule #3 Thoroughly check its purpose
Let’s use Scala as the language we’re using. So you’ve skimmed the code, there’s no Option.get, there are no var’s to be seen and all the names being used are informative and clear. Ship it!
But wait… What if the method does not solve the task at hand. Maybe it sends the wrong (but valid) message to the wrong place. Maybe it uses the wrong data to add records to a database. Even if all of the method names match their contents, unless you go back to the original task and look at the acceptance criteria, there is no way to know if this code review deserves the OK. I would suggest taking a look at the tests and see how these compare to the aim or goal of the task. Once you know that, you can ensure all of the tests test the right things. Easy peasy.