Advice to Code Reviewers

Thanks in part to the popularity of Github, I love that team code reviews are “a thing” now.

I started participating in code reviews in my first job out of college… granted, we didn’t have fancy support for it, I would simply email code to my mentors and wait for the feedback (keep in mind, those email address had ! characters instead of @). The systems we have now are quite helpful, but I wanted to talk about the other aspects of code reviews.

Sure, a primary goal of code reviews is better software quality, but we can’t loose sight of these two critical goals:

It is these two bullets that I would like to share what I currently think are the best ideas surrounding code reviews.

Take your Time

I mean, take your time in writing your comments. I often edit a comment three times to make sure it is kind and respectful. Since we all take pride in our code, reviews can make any of us a bit sensitive.

Make sure your suggestion is clearly explained, as hastily written comments may be misunderstood, causing unnecessary rewrites, pull requests, further reviews.

Compliment

Review comments don’t have to only be about improvements. Pat your coworker on the back, and call out the good code and ideas associated with the changes.

My new approach that I’m starting is leaving little beer emojis to indicate that I owe that teammate a beer for doing writing a particular gem, or solving a bug that has plagued me.

Elaborate!

While the top-level engineering promotions at most companies require a significant amount of mentoring, since you are really intelligent, you should get into the practice of mentoring others anyway. As such, start including more “why” explanations, including code examples, deep links into style guides, and references to existing code that may illustrate your point.

I think we’ve all been on the steeper parts of technological learning curve and received a “do it my way” review with zero justification. By explaining the “why”, not only does the code gets better, the team gets better too!

Careful. Do not go to the dark side with this suggestion and start ’splaining (I think you know what I’m referring to). Perhaps we should change the term from mentor to sensei, as one definition of that term I’ve heard is: one who is further ahead on the path; which characterizes a teacher as one who can look back along a path and say, “Watch your step there, that’s a particularly slippery spot. If you do fall, it’s okay, because I fell too.” It’s a compassionate approach to teaching, which we should all adopt.

You’re Wrong

Now that I’m experienced (I’m not quite old just yet), I can look back at my life and see how many times I was absolutely certain about many things, that I now see to be completely incorrect…laughably wrong. Sure, I should expect my experience should be more valuable now, but I still might be wrong.

You can be wrong too. Obviously, not all the time, but you can never be too sure. Don’t take this advice as an urge to limit your comments, but in the conversation that occurs, recognize your teammates perspective is valid too.

We can agree that writing code is quite subjective, almost artistic, right? This means we have many ways of express a solution to a problem in code, so be open to other approaches.

Time for a story. Every time I run the linter on my Python code, it complains bitterly about my use of map, filter and reduce, and claims I should use a comprehension. Perhaps this may date me, but these functions are my bread and butter, as they seem so clear to me. Comprehensions, on the other hand, seem verbose and clutter intention. However, I usually rewrite them as my other colleagues agree with the linter and feel it is easier to read.

Ask Questions

If you had posted some code, which of the following three comments would you rather see?

Inline this function, please.

Or:

Since the function called is a single line, and only called here, let’s inline the function.

Or:

Do you think that since this function is a single line, and only called here, would it be more understandable to inline it?

While they all state the same request, I’d argue the last one is the nicest. First, since we’ve already established that we might not have all the context and be wrong, asking a question not only softens the request, but initiates a dialog, not a response.

The other feature it brings is an ego-supporting back-door for both of you. Since this isn’t a direct command, the author may be more willing to accept the comment, but if the author responds with clarifying information why it should stay, you too, are more apt to accept it since it was just a question.

This tip is golden, and I often rephrase all my suggestions as questions.

Try the Passive Voice.

I’m sure that Mrs. McDougal told you in 8th Grade English class to never use the passive voice and to rewrite your sentence with an active participant, I feel the passive voice deflects criticism from the author and to the code.

Also substitute we for you, as in:

Add a comment to the complex algorithm you wrote.

To something like:

With such a complex algorithm, we should add a clarifying comment.

Granted, we could apply all these tips, to really begin a discussion:

With such an innovative approach to solving this problem, should we add a clarifying comment explaining it, or should we break out some of the expressions into functions that we could name to help the reader follow the logical flow?

Summary

While this may be obvious, our goal should always be to foster and grow a team, so don’t leave snarky or condescending comments. Even if you are the team lead with ultimate responsibility for code quality, outright commands may seem heavy-handed, and build underlying resentment.

Also, be careful with the jokes, and no, adding an emoji doesn’t make a dig funny.

I wrote this to start a discussion. What advice would you suggest to help further the goals of code quality as well as team building?


Thanks to Ken for reviewing this and your wisdom… oh, and for the great definition of sensei.

Date: 2017 Nov 28

Created: 2024-01-12 Fri 17:05