Feature branches are a great way to add new functionality to a project, while giving others the chance to review and improve the code.  Feature branches are usually created to address a specific issue, either implementing a new feature or a fix for a reported issue.

When performing a code review of a feature branch for an open source project, there are several key points to keep in mind:

Pre and Post

Since a feature branch is usually created to address a specific issue, you should be aware of the issue that the branch is trying to address.  When you test, you need to understand the problem and the effect it has.  It's somewhat pointless to run the code from the feature branch without having seen the issue that's being addressed.  If you're familiar with the issue from having run the original code, that's fine, but an awareness of the difference is essential to knowing whether the issue is fixed.

Run the Code

This one seems kind of obvious, but based on my experience with developers (especially inexperienced ones) doing testing, sometimes the developer of the feature branch doesn't even test his own code!  It's essential that someone runs the code before passing it, not just to make sure that the code addresses the issue, but that it doesn't introduce regressions.

This and the the prior requisite mean that you need to know what the point of the feature branch is and then run the code form the feature branch to ensure that it runs correctly and doesn't break existing behavior.

Code Review

It is important to look at the code to see how a new feature is being implemented.  In this way, potential security issues and inefficiencies can be addressed and exposed.  The code must read well: From a logical point of view, from a syntactical point of view, and from a documentation point of view.  If a first read-through of the code is difficult, it will be even more difficult to understand in the future when the contributors aren't around for discussion.  These issues should be surfaced during the review.

People often think that a read-through of the code is the entirety of a code review, and it is not. It is essential that the code be run and tested in addition to a read-through, for reasons mentioned above.

Other Notes

Remember that a feature branch is created to address a specific issue.  If, during the course of review, a new issue is discovered, it is not necessarily the responsibility of the feature branch under review to implement it.  Consider whether the issue would stand alone both before and after the feature branch.  If it would stand alone, it should be filed as a separate issue. Remember that "the perfect is the enemy of the good", where achieving perfection in a code review can often force unnecessary delays in the inclusion of otherwise good code.

Code conventions are important, and blatant ignorance of convention should be a reason to reject code on review.  It is a good idea to update the code being altered to better conform to conventions, but be aware that everyone makes typos, and missing a space or two required by code convention in a 300-line change isn't a good reason to reject an entire branch. This type of pedantry only subjects the whole project to delays, and this low-hanging fruit can be picked up by later code-quality sweeps.

Finally, keep in mind that a lot of contributions to open source are done by passers-by with an itch to scratch for your software.  Contributed code is a gift, and should be treated as such. Being kind and professional while adhering to a set of rules (whether these or set by the project for how reviews are to be completed) can set your project apart and make for more happy, frequent contributors. 

Comments

There are no comments on this post.