What's important when reviewing a team member's code?

This is an anonymous post sent in by a member who does not want their name disclosed. Please be thoughtful with your responses, as these are usually tough posts to write. Email [e-mail] if you'd like to leave an anonymous comment or if you want to ask your own anonymous question.

---

I just started my first job as a junior backend dev! I'm really curious to hear from DEV users as to what they look for when reviewing code for your team. I don't think I'm going to be the first person that's asked on my team, but I'd love any tips and tricks to know how best to review code and how it can be most helpful to that team member in question.

Comments

[Joe Mainwaring]

• Peer Review is a form of mentoring. As a senior engineer, I shouldn't be "grading" your pull request, rather I should be adding annotations that help educate you as to team patterns and practices.
• As a junior dev, I don't expect you to review code in the same way I would a senior, but your eyes are none the less valuable.
• Ask questions, even if it's about something that looks perfectly fine. It's an opportunity for you to learn more and build context around the project.
• Make room for reviewing. It's easy to get swallowed up by the work, but it's important to give time back to the team. Historically, I would allocate a portion of each day to reviews as a proactive measure.
• Try to avoid tearing someone's work to shreds in a review, as that can cause political fallout within the team. Issues divisive enough to force someone to throw away their work should be discussed and decided upon during planning.
• Understand that there's two rights: the right way, and the done way. At some point in your career, you'll likely be asked to accept code you disagree with, but you have deadlines and you won't have the time to take ownership and rewrite yourself. It's important at these times to mitigate the tech debt you've added and account for it in your backlog.
• CI/CD saves you time with reviews. The tests lessen the mental burden for my review, and I can point to them when they're failing and ask the developer to address those before I do a read-thru
• Make reviews fun. I try to always add a giphy when I approve a PR to break up the monotony of read-throughs and annotations.

[YASSINE SABRI]

Thank you for your detailed response

[Julia 👩🏻‍💻 GDE]

Give constructive feedback, like: „Did you think of doing this way? This could be more… . Is there a specific reason why you chose x over y?“

Not just: „This is bad. We should not do it this way.“

Stay neutral in tone.

[Phil Ashby]

As usual, any detailed answers will have 'it depends' at the start... 😁

That said:

• Think if you could support the code when the author has moved on? If not, why not?
• Think about how it supports the business plan (assuming you know what that is - sometimes its quite opaque!), is there an easier/neater way to achieve the goal(s)?
• If your team uses test-driven-development (or behaviour-driven-development), are the tests clear, supportable and express business needs?
• Think about how you might attack the new code as an adversary, see any overflows or logic problems? Has the code been audited (with appropriate analysis tools), does it need fuzzing to find things that humans never quite see?

The above assumes there has already been much automation to help with syntax / code style and such like applied before human review!

[K]

Not getting personal and not getting lost in the details.

It depends largely on the guidelines you have.

If there are guidelines saying, you should always use early returns and you need good reasons for not to, you should mark it as a problem when reviewing.

But if you don't have such guidelines, many things come down to taste. If you see a potential bug or edge-case, tell people, otherwise, leave their code as it is.

[Yuri Eastwood]

I guess the first point is: getting used to the existing guidelines! You should be basing any PR review on those to avoid personal views and, if they do not exist, question Senior Devs about it and write them down, maybe even document those guidelines yourself (and ask for their approval when done).

Other than that it's basically looking for business logic inconsistencies (which you'll only know if you know the context of the changes) or more wide topics, such as security and performance.

[David Cantrell]

The most important thing of all is to remember that it is not a competitive sport, it is a collaborative process. Be polite and kind in your collaboration. If you can't express your point politely and kindly then you have nothing worthwhile to say.

Use it to point out excellence as well as areas for improvement - the person upon whose code you are commenting will not be the only reader, your other colleagues will be too, and especially good practice should be pointed out to them.

[🌌 Sébastien Feugère ☔]

I would recommend this post and it’s second part. This is really helpful.

[saikumarpb]

Awesome Read, Thanks for posting it here.

[Volodymyr Yepishev]

Assert good intentions and that the author was trying their best :)

[Moeki Kawakami]

It is important to separate facts from personal opinion for psychological safety. Maybe we need a template for review comments for that. I haven't been able to do it.