Switch Statements Removed in CraftArt, CraftPotionEffectType, and CraftWorld#6
Switch Statements Removed in CraftArt, CraftPotionEffectType, and CraftWorld#6PhouLee wants to merge 9 commits intoNicMcPhee:masterfrom
Conversation
-Testing commit - Phou Lee
this chunk of code is big, we decided to extract it into the abstract class. Now both classes use the method from the abstract class.
smaller methods from it called setTicks() and shootFireballs(). This was done to make the code more readable.
were found within EntityBlaze and EntitySpider. We were able to extract the duplicated method and put it in EntityMonster. Tests were green.
out how to do this so instead we moved on to other refactorings. Next, we applied polymorphism to getEntity() in CraftEntity. We got about 1/5 of it done. All the tests seem to pass. Ran into issues with classes with no source code. We worked around this by just throwing the logic into the appropriate super class.
idea that we could use it on the CraftArt switch statements. After some time fiddling with it (and some help from Nic) we were able to construct a Hashmap that both switch statements can access. Hashmap is static so we had to use a static enclosement to populate the KV pairs.
the values instead. As a result code looks clearer.
just populated the Map there.
with the ids as the keys and the effect types as the values. Makes the code a bit easier to read.
|
Code Review: Non-code Review: |
There was a problem hiding this comment.
Why in the world do you look over all the entries in the map instead of just using get like you do in the other map you set up? This makes the lookup linear (admittedly over a very short list) instead of (nearly) constant time, which seems weird.
|
I quite like this refactoring – nice job. It would be nice if your handling of the lookup was more consistent in each of the cases (especially with the odd looping over the key set in the There's obviously some similarity here between the changes you've made. Is there a way to pull those together in some fashion, or do the |
4 participants
Note 1: In total we changed three switch statements into maps. However we do have not three different branches therefore we only have one pull request (Which may count as three different pull requests).
Note 2: We also made a branch out of our iteration one branch by mistake. This means we have the recent commits in addition to the previous commits from iteration.
Here is the URL to the changes that matter in this pull request!
The Issue:
There were two methods in CraftArt.java that were using switch statements to generate MineCraft objects and CraftBukkit objects. These statements were long and repetitive.
There was one method in CraftPotionEffectType.java that had a switch statement. There was also another method in CraftWorld that also had a switch statement.
Justification:
This changes that we made improved readability and decreased the number of lines in the classes.
BreakDown:
Our main focus was on Switch Statements and we used switch-statements to maps refactoring. We were able to remove both switch statements in NotchToBukkit() and BukkitToNotch(). We did by constructing a EnumMap Object to hold the Art objects as keys and the EnumArt objects as the values. NotchToBukkit uses the Map to get the keys while BukkitToNotch uses the Map to grab the values. This is faster than a switch statement when we grab the values in BukkitToNotch. However, this is about as fast as a switch statement when grabbing the keys.
We also removed switch statements in CraftPotionEffectType.getName() and CraftWorld.generateTree(). Was also used Map data structures here to remove the switch statement. In these two cases we only uses the Maps to get the values. Thus, it is faster than a switch statement. This made the methods not only faster but also improved readability.
We checked and there were tests for the switch statements (we checked by commenting the code to see if any tests broke). After refactoring each switch statement we ran the tests and they were all green.