DEV Community

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

Sloan the DEV Moderator on October 28, 2022

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 usuall...
Collapse
 
theaccordance profile image
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.

giphy

Collapse
 
yassinesabri profile image
YASSINE SABRI

Thank you for your detailed response

Collapse
 
yuridevat profile image
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.

Collapse
 
phlash profile image
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!

Collapse
 
kayis profile image
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.

Collapse
 
drhyde profile image
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.

Collapse
 
yurieastwood profile image
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.

Collapse
 
smonff profile image
🌌 Sébastien Feugère ☔

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

Collapse
 
saikumarpb profile image
saikumarpb

Awesome Read, Thanks for posting it here.

Collapse
 
bwca profile image
Volodymyr Yepishev

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

Collapse
 
moekidev profile image
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.