Reviewing someone else's code can be frustrating, but also a learning experience for everyone. Let's see how Stephen King might help us.

I was talking with my friend Tess Ferrandez at NDC London just over a month ago and we started talking about code reviews. She was about to go on stage, and that was the subject of her talk (I can't find a link to the recording), which was ironic because it was also the chapter I was outlining for The Imposter's Playbook, which I'm getting near to finishing.

I stood in the corner of the room, which was jam packed, as Tess offered her tips on being more understanding, empathetic, and, in a word: human when you review someone else's code (thanks to Emad Mokhtar for finding the link!) .

This is something we should talk more about!

Advice from Stephen King

Reviewing code for someone is, quite literally, an editing process. Your goal is to help them find the nugget of goodness, the purpose, if you will, of what they're trying to create. One of my favorite English teachers in college pushed me once, quipping: whatever you're trying to say, just say it already.

That was a bit forceful, but apparently what I needed at the time. It unlocked the idea of "voice" for me, and my writing dramatically improved from that point on.

I like how Stephen King describes the editing process in his book On Writing, which I've read 3 times now:

I like that, and I think we can adopt that idea in terms of code reviews:

My English teacher unlocked something in me that changed my life. My friend Dian Fayeedited a book I wrote, A Curious Moon, and would consistently blow my mind with her ability to see what I was trying to say, and push me to say it. It's more than fair to say that book would not be what it is today without Dian's patient, kind, thoughtful, and ridiculously smart feedback.

I think we can do the same for others when we review their code.

A Dedication to Plot and Scene

The plot of most books is made up of a series of scenes that the characters move through. We can think of code this way, as reviewers, except instead of characters we might be dealing with bits of data wrapped in classes or structs of some kind.

Clarity of intent, when it comes to code, is paramount if you're trying to convey why a bit of code exists. As a programmer, it's important to set the scene for your reviewer, so they know what's happening and why.

I think this is a challenge for many programmers, myself included. Consider the following bit of code. I found this online, but it could easily be something I wrote! Let's pretend we've been asked to review it, shall we?

What are your thoughts as you look over this code? What "scene" are we in and what characters are at work?

Channeling Terence Fletcher

Like any review, the code above has room for improvement. I'm sure you see things, immediately, that you don't care for, such as:

• No comments.
• Use of older JavaScript patterns.
• Possible floating point math issues.

To name just a few. Already sounds like I'm picking on this poor person, doesn't it? That's human nature. I'm absolutely not trying to nitpick here! If I'm asked for a review, I should give a review, right?

Unfortunately, I sound like this guy:

If you haven't seen Whiplash, by the way, please... change that! The professor above is Terence Fletcher, one of JK Simmons' most amazing roles. The guy is terrifying.

Let's not be terrifying. Let's see what we can do to be "divine reviewers", shall we?

We're Here to Help Clarify

If we understand that our role is to help our coder find the beautiful truth of their code, then this becomes a bit more fun! This, however, means that we need to address our inner Terence by seeking the good. Yes, we're being sugary and yes, there is a benefit to a dose of salt at times. We'll get there.

Here's a list of positive things we can do straight away:

• Does the code run?
• Do the tests pass (yes there are tests with this code)?
• Do we have opinions on how we would have written this?

If we're reviewing a PR on GitHub, we ideally have a build that will go off, running whatever tests that our coder has put in place. That should be the first order of business.

If there are no tests, which could easily be the case, then we're done with our review and we can reply in a supportive way: adding a few tests would help me review this code in a much more efficient way. It would also add nicely to our build - can you add a few that cover the basics?

Or, if you're on good terms with the coder, your reply could be a bit shorter: tests? This would be the "dose of salt" I referred to above. Submitting code without tests is a waste of time - namely ours as a reviewer - and people should be reminded just how precious time is.

Let's assume we have tests, for now. The tests are reasonably complete and demonstrate that the code works, as far as the coder is concerned. This is good and positive stuff - let's find something else that's a positive!

I like the use of a hash for the cart items:

I like this because I understand the scene, because I've read this book before (if you will): we don't want duplicate items in the cart, so having a hash helps with that. I do think a comment here would be useful, however, because a few years from now we might forget how carts work!

Comments tell our reviewers and our later selves about the scene unfolding in our code. If you take care to write comments that create a scene, you might also realize that you forgot something (happens to me all the time), or that something you've written just doesn't make sense (also happens to me all the time).

On to the characters now - or the "data". Who is in this scene, and should they be?

I'm noticing that an id is being sent in for the item, which means that identifying an item in the cart lies outside the cart itself. This might be by design, but I'm curious if the item has an identifier, like a sku, perhaps? If so, we can improve this method signature and, possibly, make the dialog a bit cleaner.

Finally, item is a bit generic and could benefit from more development - "rounded out" if you will. Let's suggest a little more clarity in terms of what's expected, using destructured assignment:

Let's stop at three bits of feedback (comments, using the item identifier, and destructuring). If we've done our job right, these suggestions might trigger a few ideas in our coder's head!

Offering Sparks of Inspiration

One of the things I didn't bring up in the first round of review is the lack of checks on the incoming data. What if there's no sku or name or the price is 0 or, worse, less than 0? How are discounts handled?

By offering the destructuring suggestion, we're nudging our coder to add details and make clear a few assumptions. Just seeing those three values coming in naturally makes you question their validity. That's the hope, anyway.

This code review might take a few rounds to get "right". It's a discovery process that should focus on helping our coder sharpen things, not write code the way we think it should be written.

Would I like to see a class here? Sure! A factory method in addition to an empty constructor that creates a cart from data passed to it? Yes again. Does this code needthat? I don't think so. It might come up in the future, and if it does, I can bring it up then. The important thing here is that our coder gets their PR in the door.

They might also ask if there is anything else they could do to improve the code. That's a tricky question because it means you think you know how to do it better, and maybe you do, but would adding patterns and refactoring things the way you like to see them really improve the code? It's working and the tests are passing!

So that's what I would say:

That's a tough one, when you're asked "how can I make this better". Answering that question highlights the gap between you and your coder, which can be discouraging. You might have a lot of experience, and flexing that can be destructive.

Conversely, your coder might have more current knowledge, or actually moreexperience than you in some areas. By offering your "improvements", you might be limiting yourself rather than learning something new, which happens to me in every code review I've ever done.

Thoughts?

Obviously this is highly subjective stuff, but you can make a massive difference in someone's career by offering careful, thoughtful code reviews. Who knows, you might learn something!

Have some thought for me on this? How would you improve my code review process 😇?