High Priority

Perspectives on code reviews may differ, but I advocate for a team-centric view. A pull request (PR) represents a collective effort to advance work, not just an individual achievement. If team members are solely focused on personal contributions, true teamwork is absent, and success becomes elusive.

For the past few years, I have prioritized reviewing PRs as soon as they're submitted. Learning to shift your focus to assist the team's success not only enhances your skills but also shows that context switching can be beneficial. When everyone adopts this approach, PRs can be reviewed and even approved within minutes.

Break your routine and promptly review your colleagues’ code. The success of the team is your success.

Making It Meaningful

I admit I've used "LGTM" and "GEFN" in my feedback, but only when I had no concerns and was ready to approve, or when there was a valid discussion regarding the adequacy of a solution. Being part of a strong team with solid values often leads to more positive experiences during reviews. However, when situations arise that require deeper feedback, it's essential to provide more detailed comments.

That said, avoid lengthy discourses. Keeping feedback concise yet approachable is key. If something is genuinely amiss, explain to the engineer why merging could lead to issues, rather than what they might expect. Questions are welcome, but they shouldn’t turn into lengthy puzzles that slow down the team’s progress. Aim for clarity and brevity in your reviews.

No Debates, Please

One of my pet peeves is the propensity for debates during PR reviews. For the love of code, let’s stop these discussions. It has been a while since I’ve encountered one, but they can be utterly frustrating—akin to a never-ending Reddit thread.

PRs are not the appropriate arena for philosophical discussions about coding practices, such as “if” versus “switch” statements or the merits of libraries like moment.js. These conversations should have been addressed during onboarding when team culture and coding standards are established. If debates persist, it reflects a deeper cultural issue within the engineering team rather than a technical one.

Philosophical arguments in PRs often indicate a lack of maturity in engineering practices and a flawed team dynamic.

Having a Strategy

Different teams and companies will have varying strategies, but I believe that the approach I developed is effective. It maximizes productivity while ensuring meaningful reviews that propel the team forward.

Establish a dedicated Slack or Teams channel for PR notifications so that all team members are informed. Set a collective goal to review PRs within a reasonable timeframe. If you’re reviewing, mark it in some way to let others know they can focus on different tasks.

Before diving into a review, wait for the Continuous Integration (CI) tests to complete. If tests fail, it’s futile to proceed with a review; the PR owner should address these issues first. Ideally, PRs should only be posted once the build is successful, but automation may not always allow for that.

Once the build passes, I check for the presence of tests, commented code, and console logs. An abundance of the latter two often indicates a rushed PR, so I suggest that the engineer tidy things up. Assess the tests for their relevance, and only after that, delve into the code itself. I avoid fixating on specific issues; the code simply needs to be logical.

If everything appears satisfactory but I still have reservations, I run the code locally or in a relevant environment.

Even saving five minutes per PR review can accumulate to significant time savings over a year.

Using Your Options Wisely

You might think that having a few choices for reviewing a PR would simplify the process, yet I frequently encounter engineers expressing frustration over this. Personally, I find it straightforward.

• Approve: If you’re satisfied with the code, approve it—even if you left a few comments. This indicates that your concerns are not significant enough to warrant changes.
• Comment: Utilize this option judiciously. It’s excellent for inquiries and confirming details.
• Request Changes: Reserve this for situations where the code will clearly fail if merged. This should be your last resort, only used when you feel strongly about potential issues.

Always keep the team's success in mind; this will guide your decision-making process.

Accepting Responsibility

To conclude, I want to emphasize the importance of accepting responsibility. Each team member must recognize that approving, delaying, or rejecting a PR entails accountability for that code, just like the PR’s author. If complications arise, you’ll be involved in addressing them. PRs are not about individual accolades; they are about the entire team, a feature, or a product. They should remain free from office politics and power struggles.

Once everyone accepts responsibility for their teammate’s code, responds to PRs promptly, and communicates effectively in their reviews, team velocity will inevitably increase.

As I’ve reiterated, code reviews encompass far more than emojis. They are a crucial pathway to success for any software development team.

Attila Vago — A software engineer striving to improve the world one line of code at a time. A lifelong enthusiast of technology, coding, and accessibility advocacy.