Latest revision as of 09:43, 22 May 2020

Main Page > Development > Development/Code review

Code review is essentially 2 or more developers discussing changes to the underlying source code of the application - which could be for the implementation of a bug fix or new feature, a refactor of some working code to optimize it, and so forth. Think of it as peer review for developers.

Every new feature and bugfix to AtoM is expected to go through code review before inclusion. This applies both to developers at Artefactual and to outside contributors.

Why code review?

Code review has a number of advantages, all of which help the AtoM project in the long term. Some of these include:

• Code quality: Getting more than one set of eyes on the project can ensure that no (or less) bugs slip through, that security vulnerabilities are found before they are a problem, and that an easier or more elegant solution is not overlooked.
• Code consistency: This is about 2 things: 1) consistent code is easier to maintain over time, and 2) it's easier for someone new to the project to learn. As AtoM grows, it becomes difficult for one person to have eyes on all of the corners of the application's code base. Code review can help ensure that consistent methods are used throughout the application, and deviations caught in advance.
• Awareness: As changes are introduced, it's helpful to get more eyes on what's changing, and make sure that more of the developers responsible for maintaining AtoM are familiar with how the application is evolving with each change.
• Knowledge sharing: For both our team, and our community contributors, there is always the opportunity to learn from our colleagues. Getting a second opinion can be a wonderful opportunity to learn something new!

The Code review process

So you have something to contribute to the AtoM project. Great! Thanks for sharing with the community!

Here's an outline of the code submission and review process:

1. Fork the Artefactual project on GitHub, and commit your changes to a branch
2. Do your own testing and initial review of your work - see our self-review checklist below
3. Open a pull request
4. Back and forth discussion with developers on the branch
5. Once clarity has been reached in the discussions, make any outstanding changes suggested by reviewers
6. Repeat 3 and 4 as necessary
7. Clean up the commit history, if necessary
8. Your branch will be merged!

Artefactual uses GitHub's pull request feature for code review. Every change being submitted to an Artefactual project should be submitted as a pull request to the appropriate repository, and the appropriate branch - in general, to the latest development branch (named qa/[verison]). A pull request being submitted for code review should only contain commits covering a related section of code. Try not to bundle unrelated changes together in one branch; it makes review harder.

If you're not familiar with forking repositories and creating branches in GitHub, consult their guide.

When to submit code for review?

Your code doesn't have to be ready yet to submit for code review! You should submit a branch for code review as soon as you want to get feedback on it. Sometimes, that means submitting a feature-complete branch, but sometimes that means submitting an early work-in-progress in order to get feedback on direction. Don't be shy about submitting early.

Review your work before submitting

Before submitting your work for review, it will help speed up the process if we know you've done everything you can in advance to ensure it's ready for a second set of eyes. Below is a personal checklist you can use to help guide your review:

• If you're a first time contributor, have you signed and submitted a Contributor's agreement?
• Does your branch contain just one logically separate piece of work, and not any unrelated changes?
• Have you followed the recommended best practices for writing your commit messages? Please see Chris Beam's excellent piece on How to write good commit messages
• Do your commit and/or pull request title(s) reference an issue related to this work?
• Is your branch up to date with the project's master branch? Have you rebased your branch against the project's master branch before submitting your pull request? Can your work be merged without conflicts?
• Does any new code in your pull request follow our coding standard?
• If the code contains changes to the database schema, have you also included a database migration? A good test is, can I upgrade an existing AtoM installation to use my new code without issues, following the project's upgrading documentation?
• If the code contains any changes that break backwards-compatibility for plugins, API clients, or themes, is the breakage necessary or do the benefits of the change justify the breakage?
• Have you tested your work with real data before submitting, to ensure it works as expected?
• If there are new user-visible strings, are they internationalized so they can be translated?
• If there are UI changes, have they been tested on different screen sizes and in different browsers?
• Have you added helpful comments in your code that clarify the purpose of your code (the why, more than the what)?

Opening the pull request

GitHub has an excellent guide on using the pull request feature.

Discussion

Discussion on pull requests is usually a back and forth process. Don't feel like you have to make every change the reviewer suggests; the pull request is a great place to have in-depth conversation on the issue.

Do make use of GitHub's line comment feature!

An example of line commenting during code review

By viewing the "Files changed", you can leave a comment on any line of the diff. This is a great way to scope discussion to a particular part of a diff. Any discussion you have in a specific part of the diff will also be automatically hidden once a change is pushed that addresses it, which helps keep the pull request page clear and readable.

When leaving line comments, make sure you leave them on the diff - not on the page for an individual commit. Any comments left on the commit will disappear from the pull request page as soon as that commit is no longer in the branch, even if the final PR includes a newer version of the commit.

Anyone can participate in code review discussions. Feel free to jump in if you have something to contribute on another pull request, even if you're not the one who opened it. The goal of code review is to get fresh perspectives and ensure we end up with the best implementation possible.

What do we look for?

It's hard to sum up all the considerations that might come into play during code review, but the reasons why we do it (listed above) help to indicate what we look for as well. Is the code the best that it could be? Are there any security vulnerabilities? Any bugs? Is it consistent with how we implement other similar elements in AtoM, and with AtoM's overall design? Is it going to be difficult to maintain? See the checklist included below, for a non-exhaustive list of the types of things we'll be looking for during code review.

We'll be checking the work against the recommendations found in the self-review checklist above. Additionally, we want to make sure that code conforms to our coding standard guidelines. This is one of the first things we will check!

Code review checklist

General

• Does this meet the feature requirements? Does it fix the bug?
• Do I understand what this is supposed to do? Can I run this code and verify it does what it says & is supposed to do?
• Has the contributor signed and submitted a Contributor's agreement?
• Is this work associated with a related issue ticket that might provide further context on the need for this change?
• Is this feature useful to the broader AtoM community, or is it specific to a narrow use case?
• Does it follow relevant standards and best practices where possible? Does it break standards-compliance?
• Is this one logical piece of work? Does it make sense to split this into multiple PRs for each sub-feature?

Architecture

• Does this duplicate functionality found elsewhere?
• Does it make sense for this to be implemented somewhere else?
• Does this add to or reduce the project's technical debt?
• Does this conflict or benefit from work being done in parallel?
• Does this have security implications? Does it add or close a security vulnerability?
• Is there a more efficient way to do this? Is the more efficient way significantly less readable?
• Is this change backwards compatible? If not, could it be?
• Does this work for upgrades? Do database changes include migrations?
• Has accessibility been considered in the implementation?

Style and syntax

• Does the code conform to our coding standard guidelines?
• Are the names accurate and readable?
• Are there massive functions, classes or files? Can they be split?
• Are errors handled? Is the error handling consistent with similar code?
• Are all user-facing strings marked for translation?

Tests

• Are there tests?
• Do the tests cover all the new functionality or fixes?
• Do the tests handle error cases?
• Is unit testing or integration testing more appropriate?
• Does this work with non-ASCII?

Cleaning up commit history

Once code review is finished or nearly finished, and no further development is planned on the branch, the branch's commit history should be cleaned up. You can alter the commit history of a branch using git's powerful interactive rebase feature. The following few criteria help outline what makes for a clean commit history:

Commits should be specific and atomic

Any single commit should cover a specific change. That change might span multiple files, but it shouldn't introduce multiple distinct functional changes unless it's impossible to separate those changes from each other. It's okay (and normal!) for a branch that makes a lot of changes to consist of many small commits, each of which makes individual small changes. This makes it easier to revert individual commits from a branch, and also makes it easier to isolate the commit that caused a particular bug with git-blame.

In development, it's common to introduce a change with a commit and then introduce multiple other commits with small fixes for that change. When getting ready for merging, those commits should all be squashed together.

Every commit should work

No individual commit should put the software in a broken state. It's fine for code to go unused (because the feature it's been introduced for isn't there yet), but no new functional issues should be introduced by a given commit. This ensures that reverting any individual commit from the branch is safe, and that git-bisect stays reliable for tracking down bugs.

Commit summaries should be concise

The commit summary (the first line of a commit message) should be short and clear. Usually, the first line of the commit message should be no more than 50 characters. Since the summary is just a summary of the change, it should be readable at a glance when looking through git history. The first line should be followed by a blank line, with following lines no more than 72 characters. This Github blog post summarizes why this commit summary format is important.

A good commit summary makes it clear a) what part of the code is changing, and b) what the change is. For example:

Clear commit summary:

     Normalize: fall back to default rule for unidentified files

Unclear commit summaries:

     Fixed some normalization bugs

     Bugfixes

The unclear messages make it hard to tell at a glance what changed, and that makes browsing the commit history harder.

Commit messages should be as detailed as they need to be (and no more)

The commit message is the rest of the commit past the first line. If a commit makes a small and obvious change, it's fine to not even have a commit message past the summary.

The commit message is your place to clarify the justification for a change. While there's no need to rehash anything that code comments already say, if there's more detail that helps a reader understand why a change was made, be as verbose as you need to! Remember: future-you (or another developer) will read this when going through the commit history to understand why a change was made. Make their life easier.

Guidelines for reviewers

In addition to checking the code, it's important to remember that code review is about human interaction - even when it's happening remotely and asynchronously. Someone was engaged enough with the AtoM project and community to invest time and effort into identifying an area for improvement, creating a proposed solution, and submitting it for review - and open source projects are nothing without the communities that support them. Even when the code cannot be merged, the effort counts - and the following guidelines aim to help you keep this in mind as you prepare your review.

• Compliment good code and reinforce good practices. Few things feel better than getting praise from a peer. Try to avoid backhanded compliments (e.g. "Great job, but..." followed by long list of change requests), as these can seem insincere
• Ask questions, and try to avoid inherent accusations. You might be missing details - and even if not, a question can help lead a contributor to identifying and solving the issue themselves, rather than telling them what they did was wrong. Remember that even "why" questions can be accusatory when stated bluntly - instead, try to uncover the underlying rationale and point contributors towards potential issues. Questions like, "What was the reasoning behind the approach you used here?" or "What do you think would happen if X thing happened here"? will be better received than "Why didn't you..." or blunt statements like "This is confusing" and "You shouldn't have ..."
• Use "I" statements whenever possible. Formulate your feedback from your point of view - e.g. I think, I suggest, It's hard for me to... In contrast, "You" statements can seem absolute and judgmental ("You didn't..." "You should have..." etc). This can make contributors defensive, and less open to feedback. When you ground your feedback in I-statements, there is room for discussion and consensus to be reached.
• Be specific, and include suggestions where relevant. A broad comment on a block of code like, "This doesn't make sense" is both unhelpful feedback and discouraging. Try to narrow on actionable areas for change requests whenever possible, and if the comment is more general (e.g. when something's overly complex, or unclear), offer suggestions when you can.
• Be humble, and explain your reasoning. A lot of change requests without some rationale can seem nit-picky. Remember, we all make mistakes! By contrast, giving some context for your change requests helps someone feel better prepared for future contributions.
• Comment on the code, not the contributor. Remember there's a person on the other end of this review - and one who may be a new contributor. We're all here because we care about the AtoM project. Be conscious of tone - if you don't have the time to review with care for the contributor, then perhaps this is not the time to be reviewing.
• Avoid pile ons. When multiple team members are reviewing a code contribution, resist the temptation to keep adding to comments already made - this can quickly feel like an attack to the contributor. In general, if someone has already pointed out a potential issue, you only need to comment if you have something new to contribute - a different potential solution, a compromise, something the original reviewer might have misunderstood, etc.

Guidelines for receiving feedback

It can be hard to have others review your work and offer feedback, even for experienced developers - and especially when first contributing to a project! Our goal in providing code review is to help your submission be as strong as it can be, for the benefit of the project. Below are a few things to keep in mind when receiving feedback.

• Remember, you are not your code. Feedback on your contribution is not a criticism on you as a human. You are still a valuable team member, and your peers are trying to help you improve. We're grateful you're here!
• Be humble and keep an open mind. Everyone's code can be improved, and there are always new things to learn, no matter how long you've been developing. Remember the purpose of code review is to improve the final product - everyone is on the same side. Try to check any defensive reactions you have and interrogate those feelings before responding.
• Code review is a discussion. In contrast to the point above, it's fine to disagree and discuss - it's possible the reviewer may have missed something, or that you have further knowledge that might change their review. In the end, you need to be willing to share your rationale politely and be willing to make compromises. If a reviewer can't understand your code without you explaining, how will others maintain it in the future?
• Ask questions if you need. If you don't understand the purpose of a piece of feedback, or what's being asked of you, don't hesitate to ask for clarification or suggestions on how to proceed.
• Use "I" statements whenever possible. When engaging in discussion on a particular point of review, formulate your responses from your point of view - e.g. I think, In my opinion, etc. "You" statements (e.g. "You don't understand," "If you'd just...") can quickly ratchet up tension in a dialog, and can shut down the capacity for compromise.
• Come willing to make changes. Code submissions are rarely one-and-done - this is why we include a code review process. You've invested time and effort and done the best you can - so of course you are attached to your contributions. However, don't let this prevent you from being willing to make changes that might result in a better end result. Make sure you are budgeting time to engage with the code review process, and try to remain open to the feedback you receive.

• Back to Development
• AtoM wiki home