Review Style Guide

Looks Good? Nope.

Rule Number One of Fight Club PullRequest... Don't "Looks good."

Rule Number Two: Don't "Looks good."

Reviews that are simply "Looks good", are often rated by customers as unhelpful. Almost anything else left as a summary comment is better. Whether that’s adding a few compliments for something done particularly well, or longer versions:

• After reviewing, can’t find any security issues or known vulnerabilities in dependencies, etc (whatever else looked for), good work!

• This looks like a solid fix. :+1:

• Seems to follow in line with the practices of the rest of the code and as such I've found no issues.

• Looks like [xxx] has [xxx] according to stated intent of the PR.

When internal, there’s often existing trust so shorter summary comments might work out, but it’s hard for them to sense the effort behind a “Looks good”

Don't feel you can add value to a review you picked up? You're able to unassign yourself in the dashboard.

What you're reviewing to find

Does the work match the description? Read the authors description thoughoughly and call it out if the work in the PR is missing anything that should be there.

Bugs. Should the changes work as the author intended?

Security Issues: Injection, Broken Authentication, Sensitive Data Exposure, XML External Entities, Broken Access Control, Security Misconfiguration, Cross-Site Scripting, Insecure Deserialization, Using Components With Known Vulnerabilities, Insufficient Logging And Monitoring (OWASP Top 10) and beyond. OWASP cheat sheets are a good place to link for explanations & ways to remedy.

Improve readability. Being a new set of eyes is useful as you're seing the code as new team members would. Everything make sense and laid out in the best way possible?

Simplification / Optimization. Is there a language or framework feature that the author didn't use? Provide a code snipet using it, or link them to a more indepth explanation.

Dead or commented out code / tests: Since the code is in version control, ask if the inactive code is there intentionally.

Has test coverage been added for the features that have been added?

Tone and Structure

Don't be a human linter, if the code needs line breaks or odd formatting? Suggest using a linter or mention it once saying "ditto throughout" instead of inlining every instance.

Use complete sentences, full words, capitalization and punctuation.

Avoid 'Just' and 'Simply' in your comments when providing direction on a change. If it's easy, it'll be apparent.

Avoid leaving comments that could read as rude or condescending.

You vs The code... Comment about the code, vs the developer that wrote it.

• "You should change this." vs "... can be improved to..."

• "You shouldn't ..." vs "It's best not to ..."

Finish your sentence / thought.

• "Is this correct?" vs "Just double-checking that those in a processing state shouldn't be considered here."

You're good at this, project confidence. Don't heavily hedge your comments.

• "I'm not sure, but it's possible that ..." vs "It looks like..."

Is this review helpful and actionable?

The closing question the customer sees in a review is "Was this helpful?" Is the review you're leaving likely to get a "Yes"?