Skip to content

Helper Method extracted CraftInventoryCustom.java#14

Open
Ottomoeller wants to merge 13 commits intoNicMcPhee:masterfrom
Ottomoeller:CraftInventoryCustomHelper
Open

Helper Method extracted CraftInventoryCustom.java#14
Ottomoeller wants to merge 13 commits intoNicMcPhee:masterfrom
Ottomoeller:CraftInventoryCustomHelper

Conversation

@Ottomoeller
Copy link
Copy Markdown

If you are reviewing this pull request you will only need to look at this one Ottomoeller@3066aba. We had some problems with other commits copying over with the pull request and that's why you can ignore any other commits made.
The Issue:
In CraftInventoryCustom.java there were two methods that were basically the same except for a few parts.

Justification:
Helps increase readability and reduces some duplicate code.

Break Down:
I extracted the method from splitStack and so takes two int values. And put in an if statement right after the if statement checking if the stack is null asking if the second int value is Interger.MAX_VALUE; which i set the j for splitWihoutUpdate. When the if statement passes then it uses the if/else from splitWithoutUpdate else it uses the if/else from splitStack. Depending on which it ran it returns that ones results.

@Athear
Copy link
Copy Markdown

Athear commented May 6, 2014

Review of non-code components:
While I think the code followed the appropriate CraftBukkit guidelines, I do not think your PR does. You need to be a little less terse in the issue an justification sections, and a little less descriptive of the minutia of your change in the breakdown. (I realize this last bit may be nitpicking, since you have a very contained change to describe, and there is a descriptive balancing act here). I'm not sure about your commit message: while it describes exactly what was done, it doesn't provide any justification itself, which the CraftBukkit people seem to want there as well.
Additionally, you don't mention any of the tests. You should mention if the existing tests still passed, even if you don't think they touch this part of the code (which you should also mention).

Review of code components:
While this does nicely represent a single, clearly defined change, and one that I think is an interesting refactoring, it does not ultimately do what you were after. The duplicate code has been moved into a helper method, as appropriate, but has not be removed. This code, consolidated as it now is, is apparent and presents possible changes more easily. I've noted one possible approach here.
I do believe, even with no mention of tests, that this code is correct. You have made efforts not to touch the actual logic of the code which, in absence of tests, helps ensure that it remains correct.

@schil227
Copy link
Copy Markdown

schil227 commented May 8, 2014

I concur with Athear, the Pull Request is rather difficult to understand. For example, the sentence "And put in an if statement right after the if statement checking if the stack is null asking if the second int value is Interger.MAX_VALUE" has to be read a few times through, and as such is a good indicator for rewriting the sentence. The justification portion could have also touched on making code modification easier. Lack of test acknowledgement would make me hesitant to accept this refactoring. Also, looking at the code, instead of having two method that sort of do the same thing, you've added a third and shrived up the other two; while there isnt anything necessarily wrong with that, introducing a third method in this case seems a bit smelly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants