Some basic refactoring

src/main/java/net/citizensnpcs/Citizens.java

@@ -2,7 +2,6 @@ package net.citizensnpcs;
 
 import java.io.File;
 import java.io.IOException;
-import java.sql.SQLException;
 import java.util.Iterator;
 
 import net.citizensnpcs.Settings.Setting;
@@ -18,11 +17,7 @@ import net.citizensnpcs.api.scripting.EventRegistrar;
 import net.citizensnpcs.api.scripting.ObjectProvider;
 import net.citizensnpcs.api.scripting.ScriptCompiler;
 import net.citizensnpcs.api.trait.TraitFactory;
-import net.citizensnpcs.api.util.DataKey;
+import net.citizensnpcs.command.CommandContext;
-import net.citizensnpcs.api.util.DatabaseStorage;
-import net.citizensnpcs.api.util.NBTStorage;
-import net.citizensnpcs.api.util.Storage;
-import net.citizensnpcs.api.util.YamlStorage;
 import net.citizensnpcs.command.CommandManager;
 import net.citizensnpcs.command.CommandManager.CommandInfo;
 import net.citizensnpcs.command.Injector;
@@ -38,7 +33,6 @@ import net.citizensnpcs.command.exception.ServerCommandException;
 import net.citizensnpcs.command.exception.UnhandledCommandException;
 import net.citizensnpcs.command.exception.WrappedCommandException;
 import net.citizensnpcs.editor.Editor;
-import net.citizensnpcs.npc.CitizensNPC;
 import net.citizensnpcs.npc.CitizensNPCRegistry;
 import net.citizensnpcs.npc.CitizensTraitFactory;
 import net.citizensnpcs.npc.NPCSelector;
@@ -51,7 +45,6 @@ import org.bukkit.ChatColor;
 import org.bukkit.command.Command;
 import org.bukkit.command.CommandSender;
 import org.bukkit.craftbukkit.CraftServer;
-import org.bukkit.entity.EntityType;
 import org.bukkit.entity.Player;
 import org.bukkit.plugin.Plugin;
 import org.bukkit.plugin.PluginLoadOrder;
@@ -66,7 +59,7 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
     private Settings config;
     private ClassLoader contextClassLoader;
     private CitizensNPCRegistry npcRegistry;
-    private Storage saves; // TODO: refactor this, it's used in too many places
+    private NPCDataStore saves;
     private NPCSelector selector;
     private CitizensTraitFactory traitFactory;
 
@@ -181,7 +174,7 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         tearDownScripting();
         // Don't bother with this part if MC versions are not compatible
         if (compatible) {
-            save(true);
+            saves.saveToDiskImmediate();
             despawnNPCs();
             npcRegistry = null;
         }
@@ -203,10 +196,8 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         registerScriptHelpers();
 
         config = new Settings(getDataFolder());
-
+        saves = NPCDataStore.create(getDataFolder());
-        setupStorage();
+        if (saves == null) {
-        if (!saves.load()) {
-            saves = null;
             Messaging.severeF("Unable to load saves, disabling...");
             getServer().getPluginManager().disablePlugin(this);
             return;
@@ -240,7 +231,7 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         if (getServer().getScheduler().scheduleSyncDelayedTask(this, new Runnable() {
             @Override
             public void run() {
-                setupNPCs();
+                saves.loadInto(npcRegistry);
                 startMetrics();
                 enableSubPlugins();
                 scheduleSaveTask(Setting.SAVE_TASK_DELAY.asInt());
@@ -281,68 +272,21 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         Editor.leaveAll();
         config.reload();
         despawnNPCs();
-        setupNPCs();
+        saves.loadInto(npcRegistry);
 
         getServer().getPluginManager().callEvent(new CitizensReloadEvent());
     }
 
-    public void save(boolean immediate) {
-        if (saves == null)
-            return;
-        for (NPC npc : npcRegistry)
-            ((CitizensNPC) npc).save(saves.getKey("npc." + npc.getId()));
-
-        if (immediate) {
-            saves.save();
-            return;
-        }
-        new Thread() {
-            @Override
-            public void run() {
-                saves.save();
-            }
-        }.start();
-    }
-
     private void scheduleSaveTask(int delay) {
         Bukkit.getScheduler().scheduleSyncDelayedTask(this, new Runnable() {
             @Override
             public void run() {
-                save(false);
+                storeNPCs();
+                saves.saveToDisk();
             }
         });
     }
 
-    // TODO: refactor
-    private void setupNPCs() {
-        int created = 0, spawned = 0;
-        for (DataKey key : saves.getKey("npc").getIntegerSubKeys()) {
-            int id = Integer.parseInt(key.name());
-            if (!key.keyExists("name")) {
-                Messaging.logF("Could not find a name for the NPC with ID '%s'.", id);
-                continue;
-            }
-            String unparsedEntityType = key.getString("traits.type", "PLAYER");
-            EntityType type = EntityType.fromName(unparsedEntityType);
-            if (type == null) {
-                try {
-                    type = EntityType.valueOf(unparsedEntityType);
-                } catch (IllegalArgumentException ex) {
-                    Messaging.logF("NPC type '%s' was not recognized. Did you spell it correctly?",
-                            unparsedEntityType);
-                    continue;
-                }
-            }
-            NPC npc = npcRegistry.createNPC(type, id, key.getString("name"));
-            ((CitizensNPC) npc).load(key);
-
-            ++created;
-            if (npc.isSpawned())
-                ++spawned;
-        }
-        Messaging.logF("Loaded %d NPCs (%d spawned).", created, spawned);
-    }
-
     private void setupScripting() {
         contextClassLoader = Thread.currentThread().getContextClassLoader();
         Thread.currentThread().setContextClassLoader(getClassLoader());
@@ -353,28 +297,6 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         // allows all plugin classes to be imported.
     }
 
-    private void setupStorage() {
-        String type = Setting.STORAGE_TYPE.asString();
-        if (type.equalsIgnoreCase("db") || type.equalsIgnoreCase("database")) {
-            try {
-                saves = new DatabaseStorage(Setting.DATABASE_DRIVER.asString(),
-                        Setting.DATABASE_URL.asString(), Setting.DATABASE_USERNAME.asString(),
-                        Setting.DATABASE_PASSWORD.asString());
-            } catch (SQLException e) {
-                e.printStackTrace();
-                Messaging.log("Unable to connect to database, falling back to YAML");
-            }
-        } else if (type.equalsIgnoreCase("nbt")) {
-            saves = new NBTStorage(getDataFolder() + File.separator + Setting.STORAGE_FILE.asString(),
-                    "Citizens NPC Storage");
-        }
-        if (saves == null) {
-            saves = new YamlStorage(new File(getDataFolder(), Setting.STORAGE_FILE.asString()),
-                    "Citizens NPC Storage");
-        }
-        Messaging.logF("Save method set to %s.", saves.toString());
-    }
-
     private void startMetrics() {
         new Thread() {
             @Override
@@ -400,6 +322,22 @@ public class Citizens extends JavaPlugin implements CitizensPlugin {
         }.start();
     }
 
+    public void storeNPCs() {
+        if (saves == null)
+            return;
+        for (NPC npc : npcRegistry)
+            saves.store(npc);
+    }
+
+    public void storeNPCs(CommandContext args) {
+        storeNPCs();
+        boolean async = args.hasFlag('a');
+        if (async)
+            saves.saveToDisk();
+        else
+            saves.saveToDiskImmediate();
+    }
+
     private boolean suggestClosestModifier(CommandSender sender, String command, String modifier) {
         int minDist = Integer.MAX_VALUE;
         String closest = "";

src/main/java/net/citizensnpcs/NPCDataStore.java

@@ -0,0 +1,99 @@
+package net.citizensnpcs;
+
+import java.io.File;
+import java.sql.SQLException;
+
+import net.citizensnpcs.Settings.Setting;
+import net.citizensnpcs.api.npc.NPC;
+import net.citizensnpcs.api.util.DataKey;
+import net.citizensnpcs.api.util.DatabaseStorage;
+import net.citizensnpcs.api.util.NBTStorage;
+import net.citizensnpcs.api.util.Storage;
+import net.citizensnpcs.api.util.YamlStorage;
+import net.citizensnpcs.npc.CitizensNPC;
+import net.citizensnpcs.npc.CitizensNPCRegistry;
+import net.citizensnpcs.util.Messaging;
+
+import org.bukkit.entity.EntityType;
+
+public class NPCDataStore {
+    private final Storage root;
+
+    private NPCDataStore(Storage saves) {
+        root = saves;
+    }
+
+    public void loadInto(CitizensNPCRegistry registry) {
+        int created = 0, spawned = 0;
+        for (DataKey key : root.getKey("npc").getIntegerSubKeys()) {
+            int id = Integer.parseInt(key.name());
+            if (!key.keyExists("name")) {
+                Messaging.logF("Could not find a name for the NPC with ID '%s'.", id);
+                continue;
+            }
+            String unparsedEntityType = key.getString("traits.type", "PLAYER");
+            EntityType type = EntityType.fromName(unparsedEntityType);
+            if (type == null) {
+                try {
+                    type = EntityType.valueOf(unparsedEntityType);
+                } catch (IllegalArgumentException ex) {
+                    Messaging.logF("NPC type '%s' was not recognized. Did you spell it correctly?",
+                            unparsedEntityType);
+                    continue;
+                }
+            }
+            NPC npc = registry.createNPC(type, id, key.getString("name"));
+            ((CitizensNPC) npc).load(key);
+
+            ++created;
+            if (npc.isSpawned())
+                ++spawned;
+        }
+        Messaging.logF("Loaded %d NPCs (%d spawned).", created, spawned);
+    }
+
+    public void remove(NPC npc) {
+        root.getKey("npc").removeKey(Integer.toString(npc.getId()));
+    }
+
+    public void saveToDisk() {
+        new Thread() {
+            @Override
+            public void run() {
+                root.save();
+            }
+        }.start();
+    }
+
+    public void saveToDiskImmediate() {
+        root.save();
+    }
+
+    public void store(NPC npc) {
+        ((CitizensNPC) npc).save(root.getKey("npc." + npc.getId()));
+    }
+
+    public static NPCDataStore create(File folder) {
+        Storage saves = null;
+        String type = Setting.STORAGE_TYPE.asString();
+        if (type.equalsIgnoreCase("db") || type.equalsIgnoreCase("database")) {
+            try {
+                saves = new DatabaseStorage(Setting.DATABASE_DRIVER.asString(),
+                        Setting.DATABASE_URL.asString(), Setting.DATABASE_USERNAME.asString(),
+                        Setting.DATABASE_PASSWORD.asString());
+            } catch (SQLException e) {
+                e.printStackTrace();
+                Messaging.log("Unable to connect to database, falling back to YAML");
+            }
+        } else if (type.equalsIgnoreCase("nbt")) {
+            saves = new NBTStorage(folder + File.separator + Setting.STORAGE_FILE.asString(),
+                    "Citizens NPC Storage");
+        }
+        if (saves == null)
+            saves = new YamlStorage(new File(folder, Setting.STORAGE_FILE.asString()), "Citizens NPC Storage");
+        if (!saves.load())
+            return null;
+        Messaging.logF("Save method set to %s.", saves.toString());
+        return new NPCDataStore(saves);
+    }
+}

src/main/java/net/citizensnpcs/command/command/AdminCommands.java

@@ -61,8 +61,7 @@ public class AdminCommands {
             permission = "admin")
     public void save(CommandContext args, CommandSender sender, NPC npc) {
         Messaging.send(sender, "<e>Saving Citizens...");
-        boolean async = args.hasFlag('a');
+        plugin.storeNPCs(args);
-        plugin.save(!async);
         Messaging.send(sender, "<e>Citizens saved.");
     }
 }

src/main/java/net/citizensnpcs/npc/CitizensNPCRegistry.java

@@ -4,9 +4,9 @@ import java.util.EnumMap;
 import java.util.Iterator;
 import java.util.Map;
 
+import net.citizensnpcs.NPCDataStore;
 import net.citizensnpcs.api.npc.NPC;
 import net.citizensnpcs.api.npc.NPCRegistry;
-import net.citizensnpcs.api.util.Storage;
 import net.citizensnpcs.npc.ai.NPCHolder;
 import net.citizensnpcs.npc.entity.CitizensBlazeNPC;
 import net.citizensnpcs.npc.entity.CitizensCaveSpiderNPC;
@@ -43,11 +43,11 @@ import org.bukkit.entity.EntityType;
 
 public class CitizensNPCRegistry implements NPCRegistry {
     private final ByIdArray<NPC> npcs = new ByIdArray<NPC>();
-    private final Storage saves;
+    private final NPCDataStore saves;
     private final Map<EntityType, Class<? extends CitizensNPC>> types = new EnumMap<EntityType, Class<? extends CitizensNPC>>(
             EntityType.class);
 
-    public CitizensNPCRegistry(Storage saves) {
+    public CitizensNPCRegistry(NPCDataStore saves) {
         this.saves = saves;
 
         types.put(EntityType.BLAZE, CitizensBlazeNPC.class);
@@ -94,7 +94,7 @@ public class CitizensNPCRegistry implements NPCRegistry {
     @Override
     public void deregister(NPC npc) {
         npcs.remove(npc.getId());
-        saves.getKey("npc").removeKey(String.valueOf(npc.getId()));
+        saves.remove(npc);
         npc.despawn();
     }
 
@@ -105,7 +105,7 @@ public class CitizensNPCRegistry implements NPCRegistry {
             NPC npc = itr.next();
             itr.remove();
             npc.despawn();
-            saves.getKey("npc").removeKey(String.valueOf(npc.getId()));
+            saves.remove(npc);
         }
     }
 

src/main/java/net/citizensnpcs/npc/entity/EntityHumanNPC.java

@@ -72,11 +72,6 @@ public class EntityHumanNPC extends EntityPlayer implements NPCHolder {
         // cancelled.
     }
 
-    @Override
-    public NPC getNPC() {
-        return npc;
-    }
-
     @Override
     public CraftPlayer getBukkitEntity() {
         if (npc == null)
@@ -84,11 +79,6 @@ public class EntityHumanNPC extends EntityPlayer implements NPCHolder {
         if (bukkitEntity != null)
             return (CraftPlayer) bukkitEntity;
         return (CraftPlayer) (bukkitEntity = new CraftPlayer(((CraftServer) Bukkit.getServer()), this) {
-            @Override
-            public void setMetadata(String metadataKey, MetadataValue newMetadataValue) {
-                server.getEntityMetadata().setMetadata(this, metadataKey, newMetadataValue);
-            }
-
             @Override
             public List<MetadataValue> getMetadata(String metadataKey) {
                 return server.getEntityMetadata().getMetadata(this, metadataKey);
@@ -103,9 +93,19 @@ public class EntityHumanNPC extends EntityPlayer implements NPCHolder {
             public void removeMetadata(String metadataKey, Plugin owningPlugin) {
                 server.getEntityMetadata().removeMetadata(this, metadataKey, owningPlugin);
             }
+
+            @Override
+            public void setMetadata(String metadataKey, MetadataValue newMetadataValue) {
+                server.getEntityMetadata().setMetadata(this, metadataKey, newMetadataValue);
+            }
         });
     }
 
+    @Override
+    public NPC getNPC() {
+        return npc;
+    }
+
     @Override
     public void h_() {
         super.h_();