Post-Commit Code Reviews

Coordinator
Jan 27, 2012 at 12:54 PM

I think we're making great progress so far and I've been really happy with the contributions.  However, in order to make sure we have the highest quality code and to facilitate knowledge sharing I think that we should have a process for requesting reviews of changes we submit.  Here's the idea, which was initially suggested by lordlothar:

Committer

  1. Check in changes as usual.
  2. Create a review issue (i.e., an issue with the component set to Review).
  3. Add the changeset link to the review (e.g., http://sando.codeplex.com/SourceControl/changeset/changes/ddc6bbaf9675) and/or attach your changes as a patch.
  4. Subscribe to changes for that issue (unless you already subscribe to all changes for Sando).
  5. If you see any comments on that issue please address them as best you can and check in the changes.
  6. Update the issue with the new changeset link.

Reviewer

  1. Anyone who wants to review patches can open a review task.
  2. Review the changes.
  3. Make any comments on the patch if you see any issues.
  4. If code is good enough or if your previous comments have all been addressed close the issue.

When to request a review

  • When you are unsure of your solution.
  • When your commit is larger than 10 lines or so.
  • If it's your first time committing to Sando.
  • If you want feedback to help improve your coding style and/or skills.
  • When you've reached a major milestone (e.g., Class X is now complete and I've added lots of tests and I want some feedback on it).

When to skip a review

  • Your commit is trivial (e.g., adding a comment or whitespace)
  • Your commit is only a partial solution (in this case consider creating a review once you've committed the rest of the solution)
  • You intend to get a review on the code as part of an entire class review later on.

Let me know what you guys think and if we should make any changes to the above process.

Coordinator
Jan 27, 2012 at 1:35 PM

Like Dave said - I initiated this change but not because I have doubts about our code quality, but because the reviews are really helpful - I use the review step in Autodesk in out sprints and it really help us to write better code and to find possible bugs during the early stages of the development process, not at the end by the QA tests. I think it would be very helpful for us and I hope you guys also benefit from this additional step

find possible bugs or issues in the early stage
Coordinator
Jan 27, 2012 at 5:56 PM

FYI, while reading this:

http://blogs.computerworlduk.com/apache-asserts/2012/01/be-lazy-be-fast/index.htm

I noticed this:

"By adopting a solid review process for all changes, a healthy open source software project does not require most changes to be actively pre-approved - that would be cripplingly slow. Instead, projects adopt a process that assumes an absence of objection is the same as approval. This is a safe assumption to make since changes are reversible and thus "after the event" consensus is acceptable. In the Apache Software Foundation we call this "Lazy Consensus" but you will find the same process in many other open source projects and foundations."

The takeaway point for us: reviews and code quality are important but committing new code is almost always more important. 

Luckily for us this project has several very active committers and so having new code created will probably not be a problem ;)