Reviewing with PullRequest

How do code reviews appear to the client?

PullRequest reviews will appear to the code author(s) as inline and pull/merge request summary comments - just like you would see from a team member in something like a GitHub or Bitbucket repository. In almost all cases it's best to think of yourself, the reviewer, as an extension of their development team at the code review level.

Example:

How long will I be assigned a code review?

Generally code reviews remain assigned until the code changes the client has made are merged or closed. This provides an opportunity to maintain a dialog with the code author(s).

If you come across a potential issue, but need a little more info, feel free to ask questions or for clarification.

Example:

The average length of time a code review remains active varies from team to team and, of course, the scope of the proposed code changes. Some can remain open for a few days while others only a few hours. Generally PullRequest review does not block the client's ability to merge (although some teams enforce it) so in more rare situations a code review may be merged or closed before you're able to complete a review for it.

What kinds of things should I be reviewing for?

Security vulnerabilities, logical errors, opportunities to reduce technical debt, issues with architecture, opportunities to replace methods with others which are better suited for the task, verbose operations that can be refactored, best practice adherence, typos and everything in-between!

We also encourage pointing out things that were done especially well. This reinforces good practices and lets the code author know her/his hard work has been acknowledged.

As a general rule: it's always better to call attention to something that may be a concern than not.

Example:

Submitting a great review

Code reviews vary a great deal depending on a wide body of factors including a team's style of development, the complexity of the proposed changes, the code author's level of experience, the client's business goals, and many, many others. The best code reviewers are adaptable and tailor reviews accordingly (you're probably better at this than you think!).

Generally, here are a few things we encourage for most all reviews:

• Be generous with code examples - Code samples are overwhelmingly well-received by the teams we work with. These help lighten the code author's load for composing follow-up commits and, of course, provide a great visual aid. 
  
  
• Join existing conversations - Other members of the organization may be involved in existing dialogs on a code review. If you have useful information to offer, feel free to join in!
  
  
• Follow up on changes - Just like during any code review, it's important to check back up on changes that are made in follow-up commits. While there's generally no need to comment (unless new issues have been introduced in the changes), feel free to thank the code author for making any suggested changes if the situation calls for it.

In addition to things we've found that are generally very well-received, we've also found some practices you should try to make a conscious effort to leave out of the reviews you submit.

Things to avoid in your reviews

Generally there are a few things we ask our reviewers to avoid:

• Repeated pattern feedback and nit-picky "to-dos" - While "nit-picks", like having more descriptive variable names or keeping consistent whitespacing, are important for improving a code base's maintainability, a slew of inline comments addressing every instance makes for a code review that's hard to read. Depending on the review, these can often be addressed at a high level in a summary comment or leaving one inline comment with the line "ditto throughout."
• Frame feedback as a request, not a command - Feedback that's not intended to be a terse command has a dangerous tendency to become perceived as such. Before submitting a review take the time to ensure you're erring on the side of gentleness. Framing a suggestion or feedback as a request accomplishes the same functional goal and is always better received. (Example below)

• LGTM! - While many code reviews won't contain any major items of concern, we encourage our reviewers to add high-level value when possible. The teams we work with look to PullRequest reviewers for guidance and an opportunity to learn and produce better code. This could be things like recommending a more robust or well-maintained library to replace one that's currently being used or calling attention to code that was executed particularly well (as mentioned above). 
  
  That being said, there are some cases where a simple "Looks good!" will suffice (ie - the review consists of only a few lines of very simple changes).

Which brings us to the most important point...

Use your best judgment

When in doubt, use your best judgment. Always remember to put yourself in the position of the developer or team on the other end and consider what information you would find helpful if you were in their situation.

If you ever have any questions always feel free to contact reviewers@pullrequest.com or reach out directly to a PullRequest team member on Slack.