Converted the if/else block in getType() to a series of helper methods.

[EmmaIreland]

The issue:
There was a large if/else block in getType(). It was exclusively an if/else block that checked types of inventory.

Justification:
This method can introduce inefficiencies because the cases are always checked in the same order (by necessity). By allowing the compiler to take care of figuring out which type of IInventory is being passed in, we reduce overhead.

Breakdown:
The cases in the if/else block were separated and put into smaller helper methods. Each method took a different type of inventory and the body of the method was determined by the body of the corresponding if-statement.

Testing:
Unfortunately there are no existing tests that cover this code. However it is a relatively simple refactoring, and we are confident that it still functions correctly.

[NicMcPhee]

It looks like all the classes involved here are in org.bukkit instead of in net.minecraft. Is there any chance we could add a getTypeHelper() method to the various classes that are checked, allowing you to move this logic into those classes instead of having a zillion little methods in this class?

[EmmaIreland]

The InventoryType objects that we return in most cases are in org.bukkit.event.inventory.InventoryType, which we do not have source code for; the types of inventory are in net.minecraft. So we think we can't do what you are suggesting, unless we are misunderstanding your suggestion.

[MaxMagnuson]

Code Review:
I like the refactoring in this pull request. It is a single issue inside a class that is perfect for polymorphism due to the if else structure, and that is exactly how you went about fixing it. The change should be incorporated into the CraftBukkit project because polymorphism is a much better solution to this issue than a wall of if else statements.

Non-code Review:
The pull request fits the CraftBukkit guidelines and they accurately explain and reflect the changes in the code. The major problem I see in the pull request message is the use of refactoring language. You explained what you did using refactoring terms, but the type of refactoring you did isn't mention, that being polymorphism.

[RenjiClark]

This was a very well done refactoring, both code and non-code. No complaints, my only real comment is this is a clever way of dealing with this issue.

[Boidkan]

Do they follow the CraftBukkit guidelines?
Yes they did a nice job of following the requested format of CraftBukkit.

Is the English clear and professional?
Yes, the English is clear and professional.

Is it clear what this pull request does, and why the CraftBukkit team should seriously consider accepting the pull request?
Yes, It is clear to me why this refactoring is important and why they should accept it.

Are the commit messages clear and helpful? (git allows you to edit commit messages after the fact.)
The commit message are a bit brief, but this seems ok since they cover their bases in their pull request.

Are there commits that should perhaps be merged into a single commit (which git allows you to do after the fact).
Their commits seem reasonable.

Does the English description accurately and usefully capture the actual changes to the code?
Yes, their description is very well done.

Does the pull request appropriately use the language of refactoring (e.g., names of smells, refactorings, and patterns)?
A bit lacking in that department. However, it is still clear what they are doing.

Does the pull request describe what tests (if any) exist to support the change? (This would be important in helpful assess the associated risks.)
Yes, they state that there is no test coverage for the changes.

Does the pull request represent a single, clearly definable change?
Yes.

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?
Yes, I feel the code changes and english both are both good examples of what CraftBukkit should accept.

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?
This is a cool and hard refactoring to do. Very well done.

To what degree do you believe that the changes are correct? Why/why not?
The changes look perfect. Well done.

[NicMcPhee]

It would be nice to have a few simple tests here, especially to make sure that inheritance is being handled correctly (e.g., based on the comment about Droppers and Dispensers). That can be kind of tricky because which method gets called can depend on subtle things about what the compiler understands statically from the code.

That said, I think this is a nice refactoring and definitely a step in a good direction.