Reviewing and Critiquing Work
Depending on the size of your project, you probably have a variation on one of following types of review processes (or maybe a combination of several).
- Peer Review. We are all equals and equally able to review code and accept it to the project. We learn from one-another and do our best work when we know our peers will be judging it later.
- Automated Gatekeeper. Our code has test coverage. We trust our tests and only submit work we know will pass a comprehensive test suite. Typically we ask for a second opinion before the code is pushed into the test suite (for automated deployment).
- Consensus Shepherd. Our community of coders is vigilant, and opinionated. We require consensus from interested parties before code can be marked as "reviewed by the community". We may also have a testbot which is part of our community, making it easier for human coders to know when a suggested change meets minimum standards.
- Benevolent Dictator. My code, my way. You are welcome to submit your suggestions, but I will review, or have my lieutenants review your work with a fine tooth comb. I enjoy finding your mistakes and rejecting your work. Only perfection is good enough.
At this point, the remainder of the document is focused on the peer review scenario.
Preparing to Submit Your Work
Help contributors to know at what point they are ready to submit their work for review. Your culture may lean towards show-early-show-often, or perhaps it's show-when-finished. Providing a checklist of what's expected will help increase productivity as only work that's truly ready for your specific process will be submitted for review.
Sample Review Checklist
The following are questions you may want to add to your checklist. Note: not all of the questions will apply to every work flow. Select only the relevant ones for your process.
- Does my code conform to our published standards with respect to white space, indentation, inline documentation, (etc)?
- Does my code include test coverage?
- Does my code pass all unit tests?
- Does my commit message include the ticket number for the issue I'm solving?
- If manual configuration is required to test the ticket, have I documented these steps (e.g. need to run a hook update for db schema changes)?
- Is my ticket branch up-to-date so that there will be no merge conflicts when the ticket is reviewed and rolled into the master branch?
- Have I clearly described the expected outcome of applying this ticket through a text description, screen shots, and screen cast (if relevant)?
Being a Great Reviewer
Conducting a peer review is a big responsibility. As a team member, the way you conduct your review can impact future dealings with your co-workers, and on open source projects, your review might affect whether or not a volunteer stays involved with the project. It's okay to be strict in your code review though. After all, problems introduced now by someone else, could end up being your problem in the future. When you conduct your review, be sure to keep these tips in mind:
- Limit your review to the scope of the work. An effective review is laser-focused on the problem that is being worked on. Create follow-up action items (e.g., tickets) for any out-of-scope work that you think needs additional consideration.
- Use the project's published standards to conduct your review.
- Make your review specific. Address specific lines, or components in your review. Avoid making general, sweeping statements. Make your review actionable. If it’s not immediately obvious how to implement the change you’ve requested, leave it out of your review.
- Deliver your review in a timely manner. The longer you wait to give your review, the more the creator will have to draw from their memory on why certain decisions may have been made.
- Make the review as conversational as possible. You just might learn something. My preference is to conduct peer reviews in real time so that I can go through the work with the developer. This isn’t always possible, or desirable, but it can be effective for small teams.
Conducting a Peer Review Locally
The following process is adapted from the steps Joe Shindelar developed for the Drupalize.Me team. This process is documented in the Resources section of the repository workflow-git-workshop.
Review the ticket that has been assigned to you. It should include all of the items from the "Review for Review Checklist". If it does not, ask the developer/designer to update the ticket using the checklist.
Checkout a local copy of the branch relating to the ticket. If you are working from a shared repository, use the following commands:
git fetch# downloads the branches
git branch -a# show a list of all available branches
git checkout NNNN-branch-name# no need to track, as you'll roll this branch into master if it passes review
If you are working from a forked repository, you will need to add a new remote before checking out the branch. Use the following commands (GitHub, and Bitbucket-specific commands are available from the resources at the end of this document):
git remote add <name> git://example.com/<name>/<project>.git# add the contributor's source to your list of remotes
git fetch <name># grab a list of branches for this repository
git branch -a# show a list of all available branches
git checkout -b NNNN-branch-name remotes/<name>/NNNN-branch-name# no need to track as you are going to push to your own repository after the review
- Complete any steps outlined in the ticket to confirm the branch works "as advertised". If there are any errors, or things are missing, attempt to verify if the problem is on your end (e.g. did you clear cache?). If you cannot replicate what you should be seeing, contact the developer/designer and ask them for help to get your local environment looking like what it should in the ticket.
- If you have a test suite, run the tests against the newly downloaded branch.
Once you've confirmed the branch is correct and complete it can be merged into the relevant integration branch.
Completing the Pull Request
This process assumes you have a staging environment where reviewed code can be reviewed and tested once it has been incorporated into the integration branch (in this case:
Ensure your branch is up-to-date:
git checkout dev
git checkout NNNN-branch-name
git rebase dev
Confirm the branch is still working "as advertised" per the peer review previously conducted.
Merge the branch you've reviewed into dev and push it back up.
git checkout dev
git merge --no-ff NNNN-branch-name
Complete any additional steps required to sync the database with the new code (e.g. enable new modules). Proceed only once the new code is working in the staging environment.
Leave a comment on the ticket notifying the developer that the code has been reviewed and merged into the integration branch.
Review the ticket carefully before closing it. If there are follow-up tasks outlined in the ticket, update the ticket as follows:
- update the description of the ticket so that it is clear what the next step is for the ticket
- unassign the ticket
- move the ticket to the backlog
If no follow-up actions are required, close the ticket in the ticket tracker.
- Six Ways to Make Your Peer Code Reviews More Effective
- Was It Something I Said? The Art of Giving (and getting) Actionable Critiques
- Design Criticism and the Creative Process
- The Wisdom of the Peers Mike Conley's masters thesis
- Code Review blog posts on SmartBear
- How to Run a Design Critique
- Sample Review Process
- Drupalize.Me Workflow Documentation
- Working With Pull Requests specific to Bitbucket, but provides an excellent overview of what the review process looks like from a source control perspective
- Understanding the GitHub Flow a nice visualization of what it means to complete a ticket / branch
- Merging a GitHub Pull Request Locally
- Working with Pull Requests on GitHub
- Working with Remotes
- Calculating Blame and Guilt in Git with Peer Reviews