A Guide to Code Reviews
If your are reviewing a PR, it's not just your job to confirm all tests pass, you must complete a code review. Code reviews ensure all our software conforms to high standards and exist as a very good way to pass knowledge to your team members.
It's important to remember when doing your code reviews:
- Be kind, don't leave any comments on a review you would not be happy receiving yourself.
- Avoid needlessly nitpicking. It's better to focus on issues that may cause errors or where readability is struggling. Don't annoy the developer that will have to action all these changes.
- You don't have to find flaws! It's OK to review code where you find no issues, just make sure to note that.
When reviewing code, good review comments are ones which suggest changes without being judgemental.
Asking questions like:
"Why would you do this?"
where you believe a better solution existed, will make a developer feel threatened or attacked. This could be better asked:
"In JavaScript we can use this syntax, it's a lot more readable"
Phrasing comments like this, justify why a change might be needed and you've avoided directing a question directly at the author so they can separate the critique from their own work.
Once you have been requested for a review, open the appropriate PR from the PRs Page.
With the PR open click Files Changed along the top tabs.
Here, the code that has been changed as apart of this PR will be visible. As a reviewer your are mainly focused on the additions as opposed to what has been deleted, but please check no important functionality has been removed!
When you find a line of code you would like to make a comment on, navigate to the line number in the Files Changed tab and click the Blue + button to add a comment.
A text box will appear under the line of code. Enter an appropriate code comment and click Start Review.
Continue reviewing the PR, making comments against code that you deem necessary. Once you finished making all your comments; along the top of the screen click Review Changes. When doing so, if your comments require addressing by the code author select the option Request Changes, if they do not click Approve. Then click Submit review.
If the PR was Approved skip the next section and go to straight to Merging.
If you Requested Changes be made by the code author you must wait for them to be completed. Once the author has made the appropriate changes, navigate back to the Conversation tab on the PR and scroll to the comments beneath the PR description.
The code author should have made appropriate comments in reply to yours. If the comments are appropriate and have addressed your request click Resolve Conservation.
If all your requests have been addressed then all conversations should be resolved like so:
With all changes addressed, scroll to the bottom of the PR, next to the Changes Requested title click Approve Changes to approve the PR.
With the code review complete, scroll to the bottom of the PR and click Squash and Merge.
Change the title of the squashed to commit to prefix it with Fixes [issue_number]. Now click Confirm squash & merge to merge the PR into master.
Lastly, Delete the branch to keep all forks tidy and keep branches from going out of scope.
