Home

This fork of CraftBukkit was created for the Spring, 2014, Refactoring class at the University of Minnesota, Morris (UMM). The intent is that student groups will create their own forks of this fork and "turn in" refactorings by issuing pull requests on this fork. If there are pull requests that seem likely to have upstream value we may pass them along to the original repo as pull requests.

A few useful pages:

• Creating CraftBukkit project
• Initial Smells Assignment
• First Iteration Planning
• Guidelines for contributing to CraftBukkit
  • This includes a number of important "big picture" things like a nice flowchart of how to work on the project, as well as crucial "little" details like code formatting requirements and commit message expectations. If we want to have any chance of having our work incorporated into their code base, we really have to pay attention to these kinds of things.

---

Guidelines for final code reviews

I've shared a Google spreadsheet via the mailing list that assigns everyone to review the pull requests from two different groups. (I've tried to make sure no one is assigned to their own group, or to the same group twice, but please let me know if I messed that up.)

In theory everyone has two (2) pull requests to review, but not all the requests have been submitted yet. It order to not let other people's delays cause problems for you, if one of your assigned pull requests isn't submitted when you're ready to review, then go down the list taking the next pull request that has been submitted. Hopefully this will keep people from piling up on the same few pull requests, leaving others unloved.

You should see these reviews as having two related but different parts:

• Reviewing the form and (English) language of the description of the pull request, the commit messages, and the other non-code components of the request.
• Reviewing the code of the request.

Many of the questions/criteria below will be best handled by commenting directly on the pull request. I have also created a Google Forms survey, however, to collect information on questions that don't easily map to comments one would leave directly on the pull request. You should fill out the survey for both your assigned pull requests as well as leaving comments on both those pull requests. (The URL for the survey has been sent to the mailing list.)

Reviewing the non-code components

This is perhaps more like providing feedback on a writing assignment than a typical code review. The primary goal here is to provide feedback to help ensure that the pull requests are professional. Specific comments and suggestions on any of these should be left as comments on the pull request so the group can address those issues directly.

• Do they follow the CraftBukkit guidelines? (linked above)
• Is the English clear and professional?
• Is it clear what this pull request does, and why the CraftBukkit team should seriously consider accepting the pull request?
• Are the commit messages clear and helpful? (git allows you to edit commit messages after the fact.)
• Are there commits that should perhaps be merged into a single commit (which git allows you to do after the fact).
• Does the English description accurately and usefully capture the actual changes to the code?
• Does the pull request appropriately use the language of refactoring (e.g., names of smells, refactorings, and patterns)?
• Does the pull request describe what tests (if any) exist to support the change? (This would be important in helpful assess the associated risks.)

Reviewing the code components

This part is more like a traditional code review, where the goal is to confirm that the coding changes are correct and clear. When reasonable, specific comments on the code should be left either as a comment on the pull request, or as a specific in-code comment, so the group can address the issue.

• Does the pull request represent a single, clearly definable change?
  • This doesn't mean that the pull request can only touch one (or even a few) classes/methods, but all changes should work together as a group to solve a single problem. So a half-dozen unrelated extract methods do not count as a single unified change, but a substantial group of changes needed to replace a type variable with polymorphism would.
• Do the changes in the pull request represent a meaningful change that would justify the effort of the CraftBukkit team to process and incorporate the pull request?
• To what degree do the changes in the pull request reflect an "interesting" refactoring, i.e., reflect an understanding of the concepts and techniques we've been working with this semester?
• To what degree do you believe that the changes are correct? Why/why not?
  • This one is fairly tough, especially since in many cases there aren't tests to support the change. If there are tests (either previous or new to the pull request), you should check them out and run them. If there are no appropriate tests, then you have task of reading through the code very carefully looking for any possible problems, edge cases, etc. If I find a problem (especially a fairly obvious one) in a pull request, I will hold the reviewers equally responsible for it as the coders/refactorers.