Long Method - CraftEventFactory.java#9
Conversation
due to a long elseif block from lines 423-441. Another fair chunk was
from nested if/else blocks from 396-420. The following was extracted
into the method checkMagicOrProjectile(), located at line 429:
if (damager.getBukkitEntity() instanceof ThrownPotion) {
cause = DamageCause.MAGIC;
} else if (damager.getBukkitEntity() instanceof
Projectile) {
cause = DamageCause.PROJECTILE;
}
The following was extracted into the method setCause(), located at line
439:
DamageCause cause = null;
if (source == DamageSource.FIRE) {
cause = DamageCause.FIRE;
} else if (source == DamageSource.STARVE) {
cause = DamageCause.STARVATION;
} else if (source == DamageSource.WITHER) {
cause = DamageCause.WITHER;
} else if (source == DamageSource.STUCK) {
cause = DamageCause.SUFFOCATION;
} else if (source == DamageSource.DROWN) {
cause = DamageCause.DROWNING;
} else if (source == DamageSource.BURN) {
cause = DamageCause.FIRE_TICK;
} else if (source == MELTING) {
cause = DamageCause.MELTING;
} else if (source == POISON) {
cause = DamageCause.POISON;
} else if (source == DamageSource.MAGIC) {
cause = DamageCause.MAGIC;
}
handleEntityDamageEvent() is now 32 lines.
|
Do they follow the CraftBukkit guidelines? Is the English clear and professional? Is it clear what this pull request does, and why the CraftBukkit team should seriously consider accepting the pull request? Are the commit messages clear and helpful? (git allows you to edit commit messages after the fact.) Are there commits that should perhaps be merged into a single commit (which git allows you to do after the fact). Does the English description accurately and usefully capture the actual changes to the code? Does the pull request appropriately use the language of refactoring (e.g., names of smells, refactorings, and patterns)? Does the pull request describe what tests (if any) exist to support the change? (This would be important in helpful assess the associated risks.) Does the pull request represent a single, clearly definable 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? 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? To what degree do you believe that the changes are correct? Why/why not? |
There was a problem hiding this comment.
You don't really need cause as an argument here since you never use the value that's passed in. It's really just a local variable inside checkMagicOrProjectile that happens to have the same name as the variable out here.
|
This doesn't really do a lot. From your description I'd expected more extraction and improvement than the one little method. It's a nice little extraction, to be sure, just not a major deal. (And it has an unnecessary argument as I described in the note on the code.) |
4 participants
Extracted chunks of code into separate methods in CraftEventFactory.java. The extractions were done by hand to avoid formatting issues with automated "extract method" tools.
This code has no test coverage at all.
This is a change that CraftBukkit should seriously consider accepting because there was little readability in the code before. Now, there are chunks of logic that are appropriately relocated into their own methods. This makes the code much more readable and maintainable.