Taro Logo
44

How to give better code reviews without full business context?

Profile picture
Junior Software Engineer at Microsoft2 years ago

My team owns a really wide set of features spanning across multiple repos and programming languages (C++, React, Angular). On top of that, each person works on pretty different areas (telemetry/data, UI components, client side api, etc.). We have a v1 of the app being maintained and a v2 currently being developed with some people working on v1 and some working on v2. It's impossible to know all the context behind every pull request.

How can I give quality code reviews? At most I can only provide feedback to simple code logic and really well written PR descriptions. Should I be trying to private message the developer for each PR to understand more, or is this just something that comes with time?

2.6K
1

Discussion

(1 comment)
  • 54
    Profile picture
    Tech Lead @ Robinhood, Meta, Course Hero
    2 years ago

    Great questions! I love how you're taking code review so seriously as an earlier-in-career engineer: It's something I feel like a lot of junior engineers should invest in more. I was the top code reviewer in my org at Instagram (i.e. everyone underneath my director) with 700+ reviews per half, so being able to thoroughly review PRs outside of my immediate domain was crucial.

    First, I highly recommend watching this video: 3 Ways To Write Deeper, More Impactful Code Review Comments

    At most I can only provide feedback to simple code logic and really well written PR descriptions.

    This might have more value than you think! You also shouldn't hold yourself to that high a bar as you're just starting your SWE career. I left feedback in this domain a lot and it can be very effective assuming your team has a good feedback culture and you are polite about it. Some tactics to try here:

    1. Ask for a PRD (product requirements document) if you want more context on what the PR does - It's likely they have a doc and just need to link it.
    2. Ask if a certain test case was tried if it's missing from the test plan (you can figure this out without understanding the code at all, assuming you understand the goal of the PR). It can be something as simple as, "Hey, did you try the case where the user enters in an invalid email? Just want to make sure that's fully covered as I've seen it happen a lot."

    Should I be trying to private message the developer for each PR to understand more...

    I can see this getting annoying (you are requesting time, which is the most important resource). For 95% of cases, just lean towards asking great questions on the PR. For doing a meeting, I would only do it if:

    1. You have a good relationship with this engineer.
    2. You are 110% committed towards thoroughly reviewing their PR (and all stacked/dependent PRs they have on top of it). It would be pretty bad if you requested this time and didn't leave a deep review (or didn't leave one at all).

    All that being said, here's the additional advice I have about "breaking into" code reviews that are less within your space:

    • Specialize - Find 1 area of the code that you don't really know but want to get better at. Only review extra PRs from that domain until you're quite comfortable with it (this will take a couple months at least). One of the biggest traps junior engineers fall into is spreading themselves too thin and getting scatter-brain from trying to learn too many things at once.
    • Prioritize earlier PRs in the chain - I assume your team is following the concepts here: One Diff, One Thesis - What Every Code Review Should Be Like - If not, this is something to try and improve among your team. So let's say this big feature your teammate is working on will take 25 PRs to complete. You want to start reviewing from the first PRs they submit, ideally the literal first one. This lets you "go on a journey" with that teammate as they build out their project and you have all the context as it keeps building on top of each other with each subsequent PR. Jumping into the middle of a PR chain is super hard (e.g. jumping in at PR #15 out of 25) - There's just so much backloaded context you need to absorb to leave genuinely useful feedback.