Code Reviews

#summary Policy for ensuring that commits undergo code review.

In order to ensure that commits to the k framework are of high quality, we have instituted a policy of code review in which all users who commit code will have their commits reviewed by certain parties for quality, documentation, and test coverage. In the interests of clarity, this document will provide a formal description of our policies concerning code reviews. Everyone who commits to the k framework repository is expected to be familiar with this document and follow the guidelines outlined herein. For the beginning, we do not intend to enforce that individuals perform each code review, but if that leads to commits going unreviewed and committers not pulling their weight in the code review process, we will have to revisit that issue.

= Reviewer responsibilities =

When it is your turn to review, you are expected to use Google Code's code review tools to submit a code review of each commit that it is your responsibility to review. If you are not familiar with the process of code review in general or with Google Code's features for code review specifically, please refer to the documentation on the Google Code support wiki [http://code.google.com/p/support/wiki/CodeReviews here].

More specifically, there are three areas that we as a research group expect reviewers of code to address in their code reviews:

• Code quality: If the commit you are reviewing contains portions of the code that you have issue with and believe should be changed in order to improve style, consistency, or code quality, you should say so. Use your best judgment here. Try to explain why the change you are suggesting is needed. If the person being reviewed disagrees with your reasoning, listen to his argument, but don't be swayed if you don't find it compelling. Unless the person being reviewed can make a compelling case why the code is better in the form it already is in, it is their responsibility to implement the changes suggested by the reviewer.
• Code documentation: The goal of writing documentation in the source code is to make it clear what the code does and how it works to a newcomer who is unfamiliar with the code base. If anything particularly complicated is happening in a function, that needs to be explicitly explained in a comment. More generally however, every class should have a Javadoc explaining its purpose in the larger scheme, and every method of significance in that scheme should have a Javadoc explaining the parameters it takes, the value it returns, any exceptions of note it might throw, and how it works.
• Test coverage: As of the point writing this, as we begin to try to implement a system of code reviews, many aspects of the k framework have very poor test coverage. In part this is due to limitations in our testing framework, but in significant part this is also because we have gotten into the habit of committing fixes to bugs without committing any tests that cover those bugs. Thus, we would like to address test coverage in our code reviews. Essentially, any time a commit fixes a particular piece of functionality which was broken before that commit took place, despite all tests passing, it is the job of the reviewer to ensure that a test is also committed which covers the bug and adds it to the test suite.

Because the tool will continue to have commits made on a regular basis, and because reviews need to happen promptly in order to have efficacy, we expect that if it is your turn to review you will do so promptly. Code reviews can be a lengthy process spanning many days, but at each point where it is your turn to provide feedback on changes made by the committer, you should do so within 48 hours. We have no penalty currently for not doing so, and you have no obligation to provide such feedback if the code has already been sufficiently reviewed by other parties, but you are free to add any comments you wish even if other parties have already reviewed the code.

= Committer responsibilities =

In order for this system to work, committers have to be willing to make needed changes requested by reviewers. As such, it is the responsibility of the committer to keep an open mind when people are reviewing their code. Consider not only how your commit affects you, but also how it affects the other members of the team, and how it might one day affect another team member working on the same portion of the tool. Understand that nobody's code is perfect, that requests for modifications are made in good faith and do not represent a lack of respect for your coding ability, and that we all have the ability to learn to be better programmers from each other. If a reviewer requests a change, and you have no overriding objection to making that change, you should make it. If you do think that the original form of the code is justified, you should say so in the review. If the reviewer can be persuaded, so much the better. Add a comment providing your justification to the code itself and move on. If the reviewer can persuade you, make the change and move on. Otherwise, continue to discuss the issue until some kind of consensus is reached. If no consensus can be reached, bring the issue before the group as a whole for arbitration and discussion in a K meeting.

= Commit ownership = In order to decide who has responsibility to review commits, two mechanisms are in place. The first is reviewer responsibility. Each person who commits will have at least one person on the team assigned to review their code. For a junior member of the team, this will be their mentor who helps them determine what to work on and how to proceed when difficulties arise. But senior members of the team will also be assigned to each other in order to ensure that all commits have at least one reviewer.

The process by which reviewers are assigned to each other will be managed automatically by a file called reviewers.txt in the root of the trunk of the repository. There will be a mechanism in place to inform the k list if someone makes a commit who is not recognized by the automated system. If this occurs, whoever it is should be added to reviewers.txt so that code review requests sent to the entire k list will be rare.

details of file format to follow

The second mechanism for determining who is responsible to review each commit is code ownership. Each directory in the repository will be assigned an owner whose responsibility is to review changes which include modifications to files in that directory. A commit is the responsibility of all owners of all files it modifies. Owners will be decided upon by the group based on the principle that if you frequently modify multiple files in that package, you should be an owner of that package.

Owners will be tracked automatically in the repository by means of a file called owners.txt in every relevant directory in the repository. This file will contain a list of email addresses, one per line, delineating the owners to email when a change is made. A mechanism will be in place to ensure that if a particular directory has no owners.txt, it will be assigned the owners of its parent directory. This will ensure that all files have owners, because the root of the repository will contain a file owners.txt which will email the k list. If this happens unintentionally, it will be the job of the committer to add an owners.txt file to the appropriate directory so that sending code review requests to the entire k list will be rare.

= Conclusion =

In summary, every commit to the repository will be reviewed by at least one person: specifically, by all people assigned to review the committer's code, and by all owners of files modified by the commit. These people will be notified automatically and should review the code within 48 hours, even if simply to say "looks good to me".