From 01fc296fefdfc5c3c829f13e928f7ab920c4c6cb Mon Sep 17 00:00:00 2001 From: Wesley Wolfe Date: Fri, 18 Jan 2013 02:21:38 -0600 Subject: [PATCH] Improve the item meta deserialization code-style Fixed the ItemMetaFireworkTest Add set power unit tests for FireworkMeta --- .../craftbukkit/inventory/CraftMetaBook.java | 5 - .../inventory/CraftMetaCharge.java | 6 - .../inventory/CraftMetaEnchantedBook.java | 6 - .../inventory/CraftMetaFirework.java | 6 - .../craftbukkit/inventory/CraftMetaItem.java | 107 +++++++----------- .../inventory/CraftMetaLeatherArmor.java | 8 +- .../craftbukkit/inventory/CraftMetaMap.java | 5 - .../inventory/CraftMetaPotion.java | 5 - .../craftbukkit/inventory/CraftMetaSkull.java | 5 - .../craftbukkit/inventory/ItemMetaTest.java | 36 ++++++ .../inventory/ItemStackFireworkTest.java | 4 +- 11 files changed, 81 insertions(+), 112 deletions(-) diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java index a2a91decc4..0a65280ecd 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java @@ -259,9 +259,4 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta { return builder; } - - @Override - SerializableMeta.Deserializers deserializer() { - return SerializableMeta.Deserializers.BOOK; - } } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaCharge.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaCharge.java index 40ac8adfb8..bff3be9dc7 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaCharge.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaCharge.java @@ -8,7 +8,6 @@ import org.bukkit.FireworkEffect; import org.bukkit.Material; import org.bukkit.configuration.serialization.DelegateDeserialization; import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta; -import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta.Deserializers; import org.bukkit.inventory.meta.FireworkEffectMeta; import com.google.common.collect.ImmutableMap.Builder; @@ -126,9 +125,4 @@ class CraftMetaCharge extends CraftMetaItem implements FireworkEffectMeta { return builder; } - - @Override - Deserializers deserializer() { - return Deserializers.FIREWORK_EFFECT; - } } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaEnchantedBook.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaEnchantedBook.java index 4dd394442f..757c9d44e7 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaEnchantedBook.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaEnchantedBook.java @@ -8,7 +8,6 @@ import net.minecraft.server.NBTTagCompound; import org.bukkit.Material; import org.bukkit.configuration.serialization.DelegateDeserialization; import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta; -import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta.Deserializers; import org.bukkit.enchantments.Enchantment; import org.bukkit.inventory.meta.EnchantmentStorageMeta; @@ -123,11 +122,6 @@ class CraftMetaEnchantedBook extends CraftMetaItem implements EnchantmentStorage return builder; } - @Override - Deserializers deserializer() { - return Deserializers.ENCHANTED; - } - boolean isEnchantedEmpty() { return !hasStoredEnchants(); } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaFirework.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaFirework.java index 5d955ef2d3..3f9e4a38e6 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaFirework.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaFirework.java @@ -17,7 +17,6 @@ import org.bukkit.configuration.serialization.DelegateDeserialization; import org.bukkit.craftbukkit.inventory.CraftMetaItem.ItemMetaKey.Specific; import org.bukkit.craftbukkit.inventory.CraftMetaItem.ItemMetaKey.Specific.To; import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta; -import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta.Deserializers; import org.bukkit.inventory.meta.FireworkMeta; import com.google.common.collect.ImmutableList; @@ -314,11 +313,6 @@ class CraftMetaFirework extends CraftMetaItem implements FireworkMeta { return builder; } - @Override - Deserializers deserializer() { - return Deserializers.FIREWORK; - } - @Override public CraftMetaFirework clone() { CraftMetaFirework meta = (CraftMetaFirework) super.clone(); diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java index ea6efcaa43..519d900e08 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java @@ -4,6 +4,8 @@ import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -86,79 +88,55 @@ class CraftMetaItem implements ItemMeta, Repairable { public static class SerializableMeta implements ConfigurationSerializable { static final String TYPE_FIELD = "meta-type"; - enum Deserializers { - BOOK { - @Override - CraftMetaBook deserialize(Map map) { - return new CraftMetaBook(map); - } - }, - SKULL { - @Override - CraftMetaSkull deserialize(Map map) { - return new CraftMetaSkull(map); - } - }, - LEATHER_ARMOR { - @Override - CraftMetaLeatherArmor deserialize(Map map) { - return new CraftMetaLeatherArmor(map); - } - }, - MAP { - @Override - CraftMetaMap deserialize(Map map) { - return new CraftMetaMap(map); - } - }, - POTION { - @Override - CraftMetaPotion deserialize(Map map) { - return new CraftMetaPotion(map); - } - }, - ENCHANTED { - @Override - CraftMetaEnchantedBook deserialize(Map map) { - return new CraftMetaEnchantedBook(map); - } - }, - FIREWORK { - @Override - CraftMetaFirework deserialize(Map map) { - return new CraftMetaFirework(map); - } - }, - FIREWORK_EFFECT { - @Override - CraftMetaCharge deserialize(Map map) { - return new CraftMetaCharge(map); - } - }, - UNSPECIFIC { - @Override - CraftMetaItem deserialize(Map map) { - return new CraftMetaItem(map); - } - }; + static final ImmutableMap, String> classMap; + static final ImmutableMap> constructorMap; - abstract CraftMetaItem deserialize(Map map); + static { + classMap = ImmutableMap., String>builder() + .put(CraftMetaBook.class, "BOOK") + .put(CraftMetaSkull.class, "SKULL") + .put(CraftMetaLeatherArmor.class, "LEATHER_ARMOR") + .put(CraftMetaMap.class, "MAP") + .put(CraftMetaPotion.class, "POTION") + .put(CraftMetaEnchantedBook.class, "ENCHANTED") + .put(CraftMetaFirework.class, "FIREWORK") + .put(CraftMetaCharge.class, "FIREWORK_EFFECT") + .put(CraftMetaItem.class, "UNSPECIFIC") + .build(); + + final ImmutableMap.Builder> classConstructorBuilder = ImmutableMap.builder(); + for (Map.Entry, String> mapping : classMap.entrySet()) { + try { + classConstructorBuilder.put(mapping.getValue(), mapping.getKey().getDeclaredConstructor(Map.class)); + } catch (NoSuchMethodException e) { + throw new AssertionError(e); + } + } + constructorMap = classConstructorBuilder.build(); } private SerializableMeta() { } - public static ItemMeta deserialize(Map map) { + public static ItemMeta deserialize(Map map) throws Throwable { Validate.notNull(map, "Cannot deserialize null map"); String type = getString(map, TYPE_FIELD, false); - Deserializers deserializer = Deserializers.valueOf(type); + Constructor constructor = constructorMap.get(type); - if (deserializer == null) { + if (constructor == null) { throw new IllegalArgumentException(type + " is not a valid " + TYPE_FIELD); } - return deserializer.deserialize(map); + try { + return constructor.newInstance(map); + } catch (final InstantiationException e) { + throw new AssertionError(e); + } catch (final IllegalAccessException e) { + throw new AssertionError(e); + } catch (final InvocationTargetException e) { + throw e.getCause(); + } } public Map serialize() { @@ -516,7 +494,7 @@ class CraftMetaItem implements ItemMeta, Repairable { public final Map serialize() { ImmutableMap.Builder map = ImmutableMap.builder(); - map.put(SerializableMeta.TYPE_FIELD, deserializer().name()); + map.put(SerializableMeta.TYPE_FIELD, SerializableMeta.classMap.get(getClass())); serialize(map); return map.build(); } @@ -553,11 +531,6 @@ class CraftMetaItem implements ItemMeta, Repairable { builder.put(key.BUKKIT, enchants.build()); } - @Overridden - SerializableMeta.Deserializers deserializer() { - return SerializableMeta.Deserializers.UNSPECIFIC; - } - static void safelyAdd(Iterable addFrom, Collection addTo, int maxItemLength) { if (addFrom == null) { return; @@ -584,6 +557,6 @@ class CraftMetaItem implements ItemMeta, Repairable { @Override public final String toString() { - return deserializer().toString() + "_META:" + serialize(); // TODO: cry + return SerializableMeta.classMap.get(getClass()) + "_META:" + serialize(); // TODO: cry } } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaLeatherArmor.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaLeatherArmor.java index 6cb3bb1fed..15bb43835d 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaLeatherArmor.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaLeatherArmor.java @@ -46,6 +46,7 @@ class CraftMetaLeatherArmor extends CraftMetaItem implements LeatherArmorMeta { setColor(SerializableMeta.getObject(Color.class, map, COLOR.BUKKIT, true)); } + @Override void applyToItem(NBTTagCompound itemTag) { super.applyToItem(itemTag); @@ -63,6 +64,7 @@ class CraftMetaLeatherArmor extends CraftMetaItem implements LeatherArmorMeta { return !(hasColor()); } + @Override boolean applicableTo(Material type) { switch(type) { case LEATHER_HELMET: @@ -103,11 +105,6 @@ class CraftMetaLeatherArmor extends CraftMetaItem implements LeatherArmorMeta { return builder; } - @Override - SerializableMeta.Deserializers deserializer() { - return SerializableMeta.Deserializers.LEATHER_ARMOR; - } - @Override boolean equalsCommon(CraftMetaItem meta) { if (!super.equalsCommon(meta)) { @@ -126,6 +123,7 @@ class CraftMetaLeatherArmor extends CraftMetaItem implements LeatherArmorMeta { return super.notUncommon(meta) && (meta instanceof CraftMetaLeatherArmor || isLeatherArmorEmpty()); } + @Override int applyHash() { final int original; int hash = original = super.applyHash(); diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaMap.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaMap.java index 5bad0edf92..d8af390544 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaMap.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaMap.java @@ -132,9 +132,4 @@ class CraftMetaMap extends CraftMetaItem implements MapMeta { return builder; } - - @Override - SerializableMeta.Deserializers deserializer() { - return SerializableMeta.Deserializers.MAP; - } } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaPotion.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaPotion.java index 3d565f6412..ca024d0bfc 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaPotion.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaPotion.java @@ -252,9 +252,4 @@ class CraftMetaPotion extends CraftMetaItem implements PotionMeta { return builder; } - - @Override - SerializableMeta.Deserializers deserializer() { - return SerializableMeta.Deserializers.POTION; - } } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java index 2bef968e7c..41efa5525f 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java @@ -126,9 +126,4 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { } return builder; } - - @Override - SerializableMeta.Deserializers deserializer() { - return SerializableMeta.Deserializers.SKULL; - } } diff --git a/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java b/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java index 876cefc06d..b59b36a96b 100644 --- a/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java +++ b/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java @@ -6,6 +6,7 @@ import static org.hamcrest.Matchers.*; import java.util.Arrays; import java.util.List; +import org.bukkit.Bukkit; import org.bukkit.Color; import org.bukkit.FireworkEffect; import org.bukkit.Material; @@ -30,6 +31,41 @@ import org.junit.Test; public class ItemMetaTest extends AbstractTestingBase { + static final int MAX_FIREWORK_POWER = 127; // Please update ItemStackFireworkTest if/when this gets changed. + + @Test(expected=IllegalArgumentException.class) + public void testPowerLimitExact() { + newFireworkMeta().setPower(MAX_FIREWORK_POWER + 1); + } + + @Test(expected=IllegalArgumentException.class) + public void testPowerLimitMax() { + newFireworkMeta().setPower(Integer.MAX_VALUE); + } + + @Test(expected=IllegalArgumentException.class) + public void testPowerLimitMin() { + newFireworkMeta().setPower(Integer.MIN_VALUE); + } + + @Test(expected=IllegalArgumentException.class) + public void testPowerLimitNegative() { + newFireworkMeta().setPower(-1); + } + + @Test + public void testPowers() { + for (int i = 0; i <= MAX_FIREWORK_POWER; i++) { + FireworkMeta firework = newFireworkMeta(); + firework.setPower(i); + assertThat(String.valueOf(i), firework.getPower(), is(i)); + } + } + + private static FireworkMeta newFireworkMeta() { + return ((FireworkMeta) Bukkit.getItemFactory().getItemMeta(Material.FIREWORK)); + } + @Test public void testCrazyEquality() { CraftItemStack craft = CraftItemStack.asCraftCopy(new ItemStack(1)); diff --git a/src/test/java/org/bukkit/craftbukkit/inventory/ItemStackFireworkTest.java b/src/test/java/org/bukkit/craftbukkit/inventory/ItemStackFireworkTest.java index afacf3b7d4..40b1d19da7 100644 --- a/src/test/java/org/bukkit/craftbukkit/inventory/ItemStackFireworkTest.java +++ b/src/test/java/org/bukkit/craftbukkit/inventory/ItemStackFireworkTest.java @@ -129,7 +129,7 @@ public class ItemStackFireworkTest extends ItemStackTest { new Operator() { public ItemStack operate(ItemStack cleanStack) { FireworkMeta meta = (FireworkMeta) cleanStack.getItemMeta(); - meta.setPower(150); + meta.setPower(127); cleanStack.setItemMeta(meta); return cleanStack; } @@ -148,7 +148,7 @@ public class ItemStackFireworkTest extends ItemStackTest { new Operator() { public ItemStack operate(ItemStack cleanStack) { FireworkMeta meta = (FireworkMeta) cleanStack.getItemMeta(); - meta.setPower(200); + meta.setPower(42); cleanStack.setItemMeta(meta); return cleanStack; }