Skullrefactor

[mart2967]

in CraftSkull.java, we implemented two hasmaps to encapsulate the id/skulltypes and byte/blockface data. We can then remove two large cases. Then, by creating a method which goes from object->id, we were able to remove the other large case towers! This significantly simplifies the data. We were unable to test the class using the built in test cases, so we tested our method on our own, and it seemed to work.

[KirbieDramdahl]

Review of Non-Code Components

Do non-code components (commit messages, pull requests, and such) follow the CraftBukkit guidelines?

First line of commit message briefly explains what commit is achieving?

Sometimes. "Refactored a if-else tower in CraftEventFactory.java to be a hashmap" seems reasonable, "amended previous; removed reformatting mess and made the sourceToCause" does not.

First line of commit message contains ticket and references it appropriately?

No.

Body of commit message describes how code behaves without commit?

No.

Body of commit message describes why code without commit is a problem?

No.

Body of commit message describes how commit fixes code?

Only Once - And Only Sort Of. The "Created two Hashmaps which has some id and an object, and created a method which allows accessing the id from the object" (another bad title) commit message contains this information, but since it is never explained why anything was done, this cannot really be described as explaining how a commit fixes the code.

Body of commit message limited to 78 characters per line?

Yes.

Title of pull request provides brief summary of change?

No. There is, in fact, no title.

Title of pull request contains ticket and references it appropriately?

No.

Body of pull request contains issue?

Sort Of - Not Really. The issue is stated, but you need to dig for it. Additionally, it is probably not as detailed as desirable.

Body of pull request contains justification?

Sort Of - Not Really. Again, justification is given, but briefly, and you need to dig.

Body of pull request contains breakdown?

Sort Of - Not Really. Same issues as previous two questions. Too short, and hidden.

Body of pull request contains testing results and materials?

Sort Of - Not Really. Tests were apparently implemented, but it is not explained how. Also, "seemed to work" is not a resoundingly confident statement to make about the performance of your tests.

Body of pull request contains relevant pull requests?

Not applicable in this case.

Is the English clear and professional?

No. Why are there smiley faces in the commit messages? And "It passes the tests whether it's there or not!" is probably not a particularly professional commit message.
:)

Is it clear what this pull request does, and why the CraftBukkit team should seriously consider accepting the pull request?

Sort Of - Not Really. While I feel the purpose of the pull request is explained completely, it is very condensed, and does not even come close to meeting CraftBukkit requirements.

Are the commit messages clear and helpful?

Somewhat. If you understand what is happening, the commit messages make sense. Looking at the commit messages without first looking at the code is somewhat unclear. And again, these do not even attempt to meet CraftBukkit requirements.

Are there commits that should perhaps be merged into a single commit?

Maybe. The first two commits could probably be combined. Also, the issues being corrected in the third, fourth, and fifth commits could probably be condensed, as they contain no actual refactorings.

Does the English description accurately and usefully capture the actual changes to the code?

Somewhat. The pull request seems clear, if brief. The commit messages are somewhat muddled, for reasons explained previously.

Does the pull request appropriately use the language of refactoring?

Not Really. Refactorings are described, but not in the language of refactoring.

Does the pull request describe what tests (if any) exist to support the change?

Sort Of. The pull request claims that test were implemented, that these tests "seemed to work," and... that is about it. No description. I guess I will figure this out by looking through the code.

[KirbieDramdahl]

Review of Code Components

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

No. This pull request represents two distinct changes (note that change 2 is implemented incorrectly):

1. The replacement of the getSkullType() switch statements with the idToSkullType hashmap in CraftSkull.
2. The replacement of the getBlockFace() switch statements with the idToBlockFace hashmap in CraftSkull.

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. Referring to the numbering assigned to changes in the previous question:

1. Yes. This cleared out a lot of unnecessary code, and reduces the chances that cases will be missed if additions are made to the code.
2. Yes. Again, this cleared out a lot of unnecessary code, and reduces the chances that cases will be missed if additions are made to the code.

To what degree do the changes in the pull request reflect an "interesting" refactoring?

This is a somewhat more involved refactoring, which is interesting. However, there seems to be a lack of understanding of the significance of this refactoring. While the data is simplified, as stated, the main advantage would seem to be that it reduces the chance that cases will be missed somewhere if additional cases are added.

To what degree do you believe that the changes are correct? Why/why not?

Yes. Referring again to numbering from response to first question:

1. This appears correct. The getSkullType() switch statements have been removed, and replaced with references to the idToSkullType hashmap, which appears to account for all cases.
2. This appears correct. The getBlockFace() switch statements have been removed, and replaced with references to the idToBlockFace hashmap, which appears to account for all cases.
   However, there is a claim made in the pull request that tests were implemented. No such tests appear to exist.

[kirk0242]

Non-code review: The pull request was clear, but it did not conform to the guidelines. The pull request explained clearly what they did and why they did it. Creating a pull-request that conforms to the guidelines would not be a hard from what the group has.

Code review: The refactoring is interesting and involved. Eliminating the switches in favor of hashmaps makes the code more readable and clears up a lot of messiness. The hashmaps appear to be comprehensive and cover all the cases that were covered by the switches.

[alatterner]

Do they follow the CraftBukkit guidelines? (linked above):
No they didn't. It didn't specify any particular issues nor did it state a specific problem that it was hoping to fix.

Is the English clear and professional?
The English
Is it clear what this pull request does, and why the CraftBukkit team should seriously consider accepting the pull request?
You were making some jokes in your commit messages (joking about tests). Probably not the place for jokes. As such, no, they weren't professional.
Are the commit messages clear and helpful? (git allows you to edit commit messages after the fact.)
Stand-alone, the commit messages don't help tell the story. However, when looked at side-by-side with the code, it makes some sense.
Are there commits that should perhaps be merged into a single commit (which git allows you to do after the fact).
You should've removed or merged the branches commits. They aren't part of the issue at hand.

Does the English description accurately and usefully capture the actual changes to the code?
The English seemed fine. I think that they described the issue fine.
Does the pull request appropriately use the language of refactoring (e.g., names of smells, refactorings, and patterns)?
No use of smells so not really. They indirectly described switch statements but in very... strange ways. I would've used more appropriate refactoring language.
Does the pull request describe what tests (if any) exist to support the change? (This would be important in helpful assess the associated risks.)
Not really? There was some description of tests, whether joking or not, and they didn't commit any tests. So no.

Does the pull request represent a single, clearly definable change?
No, there are two things being fixed here, not one. As such, there isn't a single, clearly defined change.

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?
I think so. It makes the code more efficient and understandable. I believe the CraftBukkit team should at least consider it.
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?
They did very interesting refactorings. Not only did they use the removal of switch statement refactorings, but they applied it to do exception checks as well. This is a very interesting refactoring.

To what degree do you believe that the changes are correct? Why/why not?
I don't see anything that they missed in their refactorings. I believe that it is correct!

[NicMcPhee]

Do you want to throw the error from here, or return null and let the code that called this throw the error?

[NicMcPhee]

I quite like this, but the reverse lookup isn't super pretty. The Google collections class BiMap would give you a reversible map that would do exactly what you want, but more cleanly. It would also lessen your testing burden, which you obviously felt to some degree.