Refactoring: Replaced 2 Switch statements with HashMaps

[alexemmons]

Refactoring 11a: Commit 17293b1
Replaced a switch statement in generateTree() with a HashMap generateTree.
Refactoring 11b: Commit 16c83f8
Replaced a switch getDataValue() with a HashMap setDataValue.

[schum476]

This is for pull request #11b. This was also made in a google doc that was shared with Nic.

Non Code Review

1. Do they follow the CraftBukkit guidlines?
   a. Commit Message Expectations
   The commit message isn’t very descriptive. I can tell what they did; replaced a switch statement with a Hashmap, but that’s about it. They did, however, make a note on that commit GitHub. I’m not sure if that’s actually part of the commit message or an extra note added through GitHub. Either way, from this point on, I’ll always be taking both into consideration. Anyhow, there’s a lot of information in that note. From that, I can tell what they did and where they did it. I can easily infer why they did it, but they didn’t explicitly state why. The commit message expectations request a limit of 78 characters per block of text, and the note made with the commit was more than 78 characters. I personally find that trivial, but it was listed in CraftBukkit’s commit message expectations, so I figured I should mention that.
2. Is the English clear and professional?
   Yes it was. Although the note made with the commit on GitHub clearly shows what they did and where they made the changes. I also feel that the language used was appropriate and professional.
3. Is it clear what this pull request does, and why the CraftBukkit team should seriously consider accepting the pull request?
   I feel that this pull request could be a bit clearer on what it does. I can certainly tell what they did, but they weren’t explicit on why they did it. I can definitely infer why it was done, but as far as stating what the problem was, the only explicit information was “Smell: Switch Statement.” Without looking at the code, I can guess that there was a long switch statement that was taking a lot of lines in this file, but it’s not clear why this was a problem. It’s the “why” that I feel this pull request could use. They did have a good explanation on how the changes benefitted the code though.
4. Are the commit messages clear and helpful?
   I feel that what is there is clear and helpful. I personally would’ve liked a little bit more on why this was a problem (as I’ve noted before).
5. Are there commits that should perhaps be merged into a single commit?
   There is only one commit here. I think it would be nice if the note on the commit was added to the commit message though. Then it’d all be in one, focused location.
6. Does the English description accurately use the language of refactoring?
   I feel that they could’ve done a little bit better in this department. I don’t recall “Switch Statement” being a “smell” that we’ve discussed in this class before. I feel that “Long Method” would’ve sufficed in naming this particular smell.
7. Does the pull request describe what tests (if any) exist to support the change?
   Yes it does (kind of). They state in the note that “These changes are not covered by any tests.” So I say “yes” in that they made it clear that their refactorings are not tested.

Code Review

1. Do they follow the CraftBukkit guidlines?
   a. Does the change fit Bukkit’s Goals?
   i. Is this change reasonably supportable and maintainable?
   Yes it is. They removed a large chunk of code from the file and compressed it down to a few lines of putting values in a HashMap and just getting the value from that. This should make the code much more maintainable.
   ii. Is this change future proof?
   While I do not like the wording of this statement, I feel that these changes are reasonably “future proof.” As stated previously, the long switch case statement was compressed into putting values into a HashMap and calling .get() on said HashMap. It’s a lot more “future proof” than the switch statement that used to be there.

   b. Code Requirements (formatting such as tabs, white spaces, etc.)
   I only scanned through the code, but it appears that the formatting in terms of tabs, white space, etc. fits according to CraftBukkit’s requests.

2. Does the pull request represent a single, clearly definable change?
   Yes it does. This pull request clearly has only one change made to it; replacing a switch statement with a HashMap.

3. 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 it does. I feel that this is a meaningful change that does improve the readability, complexity, and maintainability of the code, and I feel that this change does justify the effort of the CraftBukkit team to incorporate this change.

4. To what degree do the changes in the pull request reflect an “interesting” refactoring?
   I feel that the changes made in this pull request reflect a semi-“interesting.” The refactoring in question here wasn’t particularly “interesting” in that Nic was hoping that people would try something like polymorphism or reflection for the second pull request. But it was more interesting and required more effort than simply extracting a chunk of code into a method within the same file, or deleting commented out code.

5. To what degree do you believe that the changes are correct? Why/why not?
   It’s hard to determine for sure because of the lack of code coverage, but I feel that these changes are correct. All of the values from the switch statement were put into the HashMap, and I trust that the .get() method for HashMaps would work just fine.

[alatterner]

11b.

Noncode review:
Craftbukkit guidelines: The commit message lengthiness is not followed. Likewise, the initial commit message was not describing the specific problem but rather the solution. Even the appended commit message doesn't follow the idea of specifying the specific problem being addressed. Why was the initial code a problem in the first place?

Englishness: I do think that you were using professional English in the appended commit message, however it would've been nice to cite actual refactoring issues. So in that regard, professional word usage was foregone.

I do believe that you made sure that the pull request was well-described and what specifically it aimed to do. You described the positives to accepting your pull request -- the benefits that your pull request offers.

The commit messages were initially unhelpful, however the appended commit message was helpful. With that said, there was only one commit message and one fix, I'm unsure as to the path that you followed. I'm assuming your process was multistep and not just "we threw this code together and it seemed to be okay." I'd like to see your thought process during these steps.

There was one commit message so it wasn't realistic to merge any commits.

The English description does describe it somewhat. However, I'd like to see reasoning as to why these changes are actually refactorings and not changes in behavior (which... there totally is changes in behavior....)

You do use the "switch statement" smell, but there are also other smells that you fixed (such as code duplication). I think that you were too conservative about the smells that you were describing, you only picked the obvious smell.

You stated that there were no tests and that is correct.

Code-Review:
You followed a single change -- fixing the switch statement. That is all you did and you didn't let your work leak elsewhere. So this pull request does exhibit a single problem fix.

In my opinion, no. This was hastily done it seems and it is reflected in the fact that you no longer check for exceptions. They had that exception there for a reason and you shouldn't remove it just because it doesn't fit with your model. Given that you changed the behavior of the code (removed an exception check), this code pull request should probably not be taken into consideration as a pull request.

I think that this refactoring focused on just a small portion of what we learned in class. We learned how to change a switch statement into a hashmap, and that's what they did. But they didn't use what we learned and went above and beyond to actually make this code that would be acceptable in industry. In my opinion, that's what this class was about. Learn the basics and learn how to apply those concepts in order to create meaningful refactorings that reflect industry-level code.

The changes are not "correct." There is no longer the exception check and, as such, you would've likely broken hypothetical tests that check for that exception. They also missed the "up" case. As such, this code is not correct.

[Paul-Schliep]

11a Review:

Non-code review:
I don't think that it follows the craft bukkit guidelines. The commit messages are not very detailed and the pull request is a little vague. I would like to see a little more detail about what you guys are actually doing in this, code wise and change wise. The English is very professional though and I'm glad it was because it makes it easier to understand what they are doing, even if it might be a little vague, content wise. Overall, I don't think that craft bukkit would accept this though just because it could be more descriptive of where and why and what the goals are of this refactoring and they need as much detail as possible to accept these pull requests.

Code Review:
I'm glad that this was a single code fix, being the switch statement. It makes it much easier to see what changes were made and how you were able to do it. In terms of being able to actually get the code to work, that's kinda iffy. I don't know that much about hashmaps and how well they work to replace the switch statements, so I can't really answer in 100 percent confidence. I would say it's probably not correct though because it looks like they replaced the switch statements with a hashmap of the statement itself, which probably doesn't really work. It's hard to tell though because there are no tests for coverage. This is a really interesting refactoring though, I like the idea of using hashmaps to replace switch statements for performance improvements and reducing complexity.

[NicMcPhee]

I quite like the idea here, but constructing the map in the method (so it gets constructed anew _every time the method is called) is just a very bad idea. Also, leaving in the old logic as commented out code just adds clutter and looks really unprofessional.

I take back my comment about commenting out the old code. You get full marks for reading their guidelines more carefully than I did and noticing that they ask you to leave code like that in, but commented out. I had missed (or forgotten) that entirely.

That said, in my experience leaving code like that in is really odd and not the norm, and people are usually super grumpy when they run across something like that. I suspect that the CraftBukkit people want to be able to see what's being deleted when they discuss the pull request (but you can in the code diffs, so it's odd), and will then remove it before finalizing the pull request.