Add test, fix bug

src/net/citizensnpcs/Citizens.java

@@ -101,9 +101,9 @@ public class Citizens extends JavaPlugin {
             NPC npc = npcManager.createNPC(key.getString("name"), character);
 
             // Load the character if it exists, otherwise remove the character
-            if (character != null)
+            if (character != null) {
                 character.load(key.getRelative(character.getName()));
-            else {
+            } else {
                 if (key.keyExists("character")) {
                     Messaging.debug("Character '" + key.getString("character")
                             + "' does not exist. Removing character from the NPC with ID '" + npc.getId() + "'.");

src/net/citizensnpcs/EventListen.java

@@ -34,16 +34,17 @@ public class EventListen implements Listener {
         if (!manager.isNPC(event.getEntity()))
             return;
 
-        event.setCancelled(true);
+        event.setCancelled(true); // TODO: implement damage handlers
         if (event instanceof EntityDamageByEntityEvent) {
             EntityDamageByEntityEvent e = (EntityDamageByEntityEvent) event;
             if (e.getDamager() instanceof Player) {
                 NPC npc = manager.getNPC(event.getEntity());
-                if (npc.getCharacter() != null)
+                if (npc.getCharacter() != null) {
                     npc.getCharacter().onLeftClick(npc, (Player) e.getDamager());
                 }
             }
         }
+    }
 
     @EventHandler
     public void onEntityTarget(EntityTargetEvent event) {
@@ -59,7 +60,8 @@ public class EventListen implements Listener {
      */
     @EventHandler
     public void onChunkLoad(ChunkLoadEvent event) {
-        for (Iterator<Integer> itr = toRespawn.iterator();; itr.hasNext()) {
+        Iterator<Integer> itr = toRespawn.iterator();
+        while (itr.hasNext()) {
             NPC npc = manager.getNPC(itr.next());
             npc.spawn(npc.getTrait(SpawnLocation.class).getLocation());
             itr.remove();

src/net/citizensnpcs/Settings.java

@@ -52,13 +52,14 @@ public class Settings {
     }
 
     private final YamlStorage config;
+    private final DataKey root;
 
     public Settings(Citizens plugin) {
         config = new YamlStorage(plugin.getDataFolder() + File.separator + "config.yml");
+        root = config.getKey("");
     }
 
     public void load() {
-        DataKey root = config.getKey("");
         for (Setting setting : Setting.values()) {
             if (!root.keyExists(setting.getPath())) {
                 Messaging.log("Writing default setting: '" + setting.getPath() + "'");

src/net/citizensnpcs/npc/CitizensNPC.java

@@ -32,7 +32,7 @@ public class CitizensNPC implements NPC {
         this.name = name;
         this.character = character;
         manager = (CitizensNPCManager) CitizensAPI.getNPCManager();
-        id = manager.generateUniqueId();
+        id = manager.getUniqueID();
     }
 
     public CitizensNPC(int id, String name, Character character) {
@@ -59,11 +59,12 @@ public class CitizensNPC implements NPC {
 
     @Override
     public void addTrait(Trait trait) {
-        if (!hasTrait(trait.getClass()))
+        if (!hasTrait(trait.getClass())) {
             traits.put(trait.getClass(), trait);
-        else
+        } else {
             Messaging.debug("The NPC already has the trait '" + getTrait(trait.getClass()).getName() + "'.");
         }
+    }
 
     @Override
     public Character getCharacter() {
@@ -133,10 +134,11 @@ public class CitizensNPC implements NPC {
             return;
         }
 
-        if (mcEntity == null)
+        if (mcEntity == null) {
             mcEntity = manager.spawn(this, loc);
-        else
+        } else {
             manager.spawn(this, loc);
+        }
 
         // Set the location
         addTrait(new SpawnLocation(loc));
@@ -160,8 +162,9 @@ public class CitizensNPC implements NPC {
 
     @Override
     public void remove() {
-        if (isSpawned())
+        if (isSpawned()) {
             despawn();
+        }
         manager.remove(this);
     }
 

src/net/citizensnpcs/npc/CitizensNPCManager.java

@@ -77,7 +77,7 @@ public class CitizensNPCManager implements NPCManager {
         return spawned.contains(entity.getEntityId());
     }
 
-    public int generateUniqueId() {
+    public int getUniqueID() {
         int count = 0;
         while (true) {
             if (getNPC(count) == null)
@@ -89,7 +89,7 @@ public class CitizensNPCManager implements NPCManager {
 
     public CraftNPC spawn(NPC npc, Location loc) {
         if (spawned.contains(npc.getBukkitEntity().getEntityId()))
-            throw new IllegalStateException("The NPC with ID '" + npc.getId() + "' is already spawned.");
+            throw new IllegalStateException("already spawned");
         WorldServer ws = getWorldServer(loc.getWorld());
         CraftNPC mcEntity = new CraftNPC(getMinecraftServer(ws.getServer()), ws, npc.getFullName(),
                 new ItemInWorldManager(ws));
@@ -104,10 +104,11 @@ public class CitizensNPCManager implements NPCManager {
 
     public void despawn(NPC npc) {
         if (!spawned.contains(npc.getBukkitEntity().getEntityId()))
-            throw new IllegalStateException("The NPC with ID '" + npc.getId() + "' is already despawned.");
+            throw new IllegalStateException("already despawned");
         CraftNPC mcEntity = ((CitizensNPC) npc).getHandle();
-        for (Player player : Bukkit.getOnlinePlayers())
+        for (Player player : Bukkit.getOnlinePlayers()) {
             ((CraftPlayer) player).getHandle().netServerHandler.sendPacket(new Packet29DestroyEntity(mcEntity.id));
+        }
         Location loc = npc.getBukkitEntity().getLocation();
         getWorldServer(loc.getWorld()).removeEntity(mcEntity);
         npc.getTrait(SpawnLocation.class).setLocation(loc);

src/net/citizensnpcs/npc/trait/CitizensTraitManager.java

@@ -13,8 +13,9 @@ public class CitizensTraitManager implements TraitManager {
 
     @Override
     public Trait getTrait(String name) {
-        if (registered.get(name) == null)
+        if (registered.get(name) == null) {
             return null;
+        }
         return registered.get(name).create();
     }
 
@@ -26,7 +27,7 @@ public class CitizensTraitManager implements TraitManager {
     @Override
     public void registerTraitWithFactory(String name, Factory<? extends Trait> factory) {
         if (registered.get(name) != null)
-            throw new IllegalArgumentException("Trait factory already registered.");
+            throw new IllegalArgumentException("trait factory already registered");
         registered.put(name, factory);
     }
 

src/net/citizensnpcs/test/ByIdArrayTest.java

@@ -0,0 +1,40 @@
+package net.citizensnpcs.test;
+
+import java.util.Random;
+
+import net.citizensnpcs.util.ByIdArray;
+
+import org.junit.Test;
+
+public class ByIdArrayTest {
+    @Test
+    public void testInsert() {
+        ByIdArray<String> test = ByIdArray.create();
+        test.put(0, "one");
+        assert (test.get(0).equals("one"));
+        assert (test.contains(0));
+        assert (test.size() == 1);
+        test.remove(1000); // try illegal remove
+        test.clear();
+        assert (test.size() == 0);
+    }
+
+    @Test
+    public void testIteration() {
+        int iterations = 1000;
+        ByIdArray<String> test = ByIdArray.create();
+        String[] values = new String[iterations];
+        for (int i = 0; i < values.length; ++i) {
+            values[i] = Integer.toString(i);
+        }
+        Random random = new Random(100);
+        int index = 0;
+        for (String value : values) {
+            test.put((index += random.nextInt(10) + 1), value);
+        }
+        index = 0;
+        for (String value : test) {
+            assert (value.equals(values[index++]));
+        }
+    }
+}

src/net/citizensnpcs/util/ByIdArray.java

@@ -8,8 +8,8 @@ public class ByIdArray<T> implements Iterable<T> {
     private Object[] elementData;
     private int size;
     private int modCount;
-    private int highest;
+    private int highest = Integer.MIN_VALUE;
-    private int lowest;
+    private int lowest = Integer.MAX_VALUE;
 
     public ByIdArray() {
         this(50);
@@ -17,7 +17,7 @@ public class ByIdArray<T> implements Iterable<T> {
 
     public ByIdArray(int capacity) {
         if (capacity < 0)
-            throw new IllegalArgumentException("Illegal capacity: cannot be below 0.");
+            throw new IllegalArgumentException("illegal capacity");
         elementData = new Object[capacity];
     }
 
@@ -46,7 +46,7 @@ public class ByIdArray<T> implements Iterable<T> {
     }
 
     private void recalcLowest() {
-        while (elementData.length > lowest && elementData[lowest++] == null)
+        while (elementData.length > lowest && highest > lowest && elementData[lowest++] == null)
             ;
     }
 
@@ -73,9 +73,9 @@ public class ByIdArray<T> implements Iterable<T> {
                 T next = (T) elementData[idx];
                 if (next == null || idx > highest)
                     throw new NoSuchElementException();
-                do
+                do {
                     idx++;
-                while (idx != highest + 1 && elementData[idx] == null);
+                } while (idx != highest + 1 && elementData[idx] == null);
                 return next;
             }
 
@@ -88,7 +88,7 @@ public class ByIdArray<T> implements Iterable<T> {
 
     public void put(int index, T t) {
         if (t == null)
-            throw new IllegalArgumentException("'t' cannot be null.");
+            throw new IllegalArgumentException("t cannot be null");
         ++modCount;
         if (index > highest)
             highest = index;
@@ -107,6 +107,8 @@ public class ByIdArray<T> implements Iterable<T> {
     }
 
     public T remove(int index) {
+        if (index > elementData.length || elementData[index] == null)
+            return null;
         ++modCount;
         if (index == lowest)
             recalcLowest();
@@ -133,6 +135,10 @@ public class ByIdArray<T> implements Iterable<T> {
         return new ByIdArray<T>();
     }
 
+    public static <T> ByIdArray<T> create(int capacity) {
+        return new ByIdArray<T>(capacity);
+    }
+
     public boolean contains(int index) {
         return elementData.length > index && elementData[index] != null;
     }