MySQL Question

Discussion in 'Spigot Plugin Development' started by Devy2013, Jun 14, 2015.

  1. Hello,
    Today I decided to man up and learn MySQL! I read numerous guides and videos about it, and then created a small little plugin. I was hoping somebody can review it for me and tell me if I'm doing anything wrong, or if it even WORKS, as I havn't even tested in yet :3.

    Code:
    PHP:
    import com.connorlinfoot.bountifulapi.BountifulAPI;
    import org.bukkit.Bukkit;
    import org.bukkit.entity.Player;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.Listener;
    import org.bukkit.event.entity.PlayerDeathEvent;
    import org.bukkit.event.player.PlayerJoinEvent;
    import org.bukkit.event.player.PlayerQuitEvent;
    import org.bukkit.plugin.java.JavaPlugin;
    import org.bukkit.scheduler.BukkitRunnable;

    import java.sql.*;
    import java.util.HashMap;
    import java.util.UUID;

    public class Main extends JavaPlugin implements Listener {

        Connection conn = null;
        PreparedStatement statement;

        private HashMap<UUID, Integer> kills = new HashMap<UUID, Integer>();
        private HashMap<UUID, Integer> deaths = new HashMap<UUID, Integer>();


        @Override
        public void onEnable() {

            getServer().getPluginManager().registerEvents(this, this);

            new BukkitRunnable() {

                @Override
                public void run() {
                    openConnection();
                }
            }.runTaskAsynchronously(this);

            Bukkit.getScheduler().scheduleSyncRepeatingTask(this, new Runnable() {
                @Override
                public void run() {
                    for (Player p : Bukkit.getOnlinePlayers()) {
                        updateActionBars(p);
                    }
                }
            }, 0, 20);
        }

        @Override
        public void onDisable() {
            for (UUID uuid : deaths.keySet()) {
                int pDeaths = deaths.get(uuid);
                int pKills = kills.get(uuid);
                String sql = "INSERT INTO stats(uuid,kills,deaths) "
                        + "VALUES(?,?,?)";
                try {
                    statement = conn.prepareStatement(sql);
                    statement.setString(1, uuid.toString());
                    statement.setInt(2, kills.get(uuid));
                    statement.setInt(3, deaths.get(uuid));
                    //Sends to database
                    statement.execute();
                    kills.remove(uuid);
                    deaths.remove(uuid);
                    statement.close();
                } catch (SQLException e1) {
                    e1.printStackTrace();
                }
            }
        }

        @EventHandler
        public void onLeave(PlayerQuitEvent e) {
            Player p = e.getPlayer();
            UUID uuid = p.getUniqueId();
            if (kills.containsKey(uuid) && deaths.containsKey(uuid)) {
                String sql = "INSERT INTO stats(uuid,kills,deaths) "
                        + "VALUES(?,?,?)";
                try {
                    statement = conn.prepareStatement(sql);
                    statement.setString(1, p.getUniqueId().toString());
                    statement.setInt(2, kills.get(uuid));
                    statement.setInt(3, deaths.get(uuid));
                    //Sends to database
                    statement.execute();
                    kills.remove(uuid);
                    deaths.remove(uuid);
                    statement.close();
                } catch (SQLException e1) {
                    e1.printStackTrace();
                }
            }
        }

        //Open MySQL Database Connection
        private synchronized void openConnection() {
            try {
                conn = DriverManager.getConnection("jdbc:mysql://localhost:3306/mysqljdbc", "username", "password");
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }

        @EventHandler
        public void onJoin(PlayerJoinEvent e) {
            Player p = e.getPlayer();
            UUID uuid = p.getUniqueId();
            String sql = "SELECT ID FROM stats WHERE ID = " + uuid.toString();
            try {
                ResultSet rs = conn.prepareStatement(sql).executeQuery();
                if (rs.isBeforeFirst()) {
                    //Column is in the database in the database
                    kills.put(uuid, rs.getInt("kills"));
                    deaths.put(uuid, rs.getInt("deaths"));
                } else {
                    //Column is not in the database in the database
                    kills.put(uuid, 0);
                    deaths.put(uuid, 0);
                }
                //Closing ResultSet
                rs.close();
            } catch (SQLException exception) {
                exception.printStackTrace();
            }
        }

        @EventHandler
        public void onDeath(PlayerDeathEvent e) {
            Player p = e.getEntity();
            Player killer = p.getKiller();
            kills.put(killer.getUniqueId(), kills.get(killer.getUniqueId()) + 1);
            deaths.put(p.getUniqueId(), deaths.get(p.getUniqueId()) + 1);
        }

        private void updateActionBars(Player p) {
            BountifulAPI.sendActionBar(p, "§aKills: §b" + kills.get(p.getUniqueId()) + " §d|| §aDeaths: §b" + deaths.get(p.getUniqueId()));
        }


    }



     
    Thanks!
     
  2. sothatsit

    Patron

    Can i ask why you have not tested it? Do you expect us to do it for you? o_O

    Anyway, here are a few things i can pick out:
    - You should run all database interaction asynchronously. Only exception being onDisable as you cannot start new asynchronous tasks when your plugin is disabled.

    - You should always use prepared statements when entering information in sql, even if you are 100% sure it will be fine. Better safe than sorry.

    - In your SELECT query you only select the ID, not the kills or deaths. I know you are supposed to use "kills, deaths" instead, but i usually just use "*" as i have had issues with "kills, deaths" not working.

    - Im not sure what this method does "rs.isBeforeFirst()" but that is not usually what i do. I use a while loop with "rs.next()" as the condition. This means that if multiple rows are selected, you will loop through multiple of them, but your uuid would be unique, so you could just use "if(rs.next())"

    - In your SELECT statement, you select the column "ID", but in your INSERT statement you set the "uuid" column, which is it?

    - You need to use INSERT only when the player is not already in the database. Otherwise you need to use UPDATE. You could just store whether you were able to retrieve the data of the player in the join, and if yes, update, else insert.

    - When making your database interaction asynchronous you will have to set some variables to final, otherwise you wont be able to access them. You could set the event to final (probably not the best), or you could retrieve the information you need in the query from the event and store them to final variables.

    - when removing the players from the kill/death maps you will need to run that synchronously, you could either run another synchronous task from inside the asynchronous task or remove them in the event and storing the information that was in them to a final variable. like "final int kills = kills.get(e.getPlayer().getUniqueId());"

    - Your PlayerDeathEvent handler will throw null pointers when a player dies without being killed (fall damage, lava, suffocation, etc...)

    - While it doesn't matter really, you should fix your formatting... Its giving me a headache xD. Things like, explicitly saying what access each variable has (private, public, protected) and not having 2 line spaces. This one does not matter really, just personal preference :p.

    Nice first try though :).
     
    • Like Like x 1
  3. Thanks for the response,
    Reason I didn't test it myself is because I'm waiting for my Sysadmin to install MySQL on my test server for me to test it. Now time to ask you about those questions / answer it. :p

    1. What did I not run asynchronously, this was the main thing I really didnt understand, mainly what it even was.

    2. I did use all prepared statements?

    3. If I use * wouldn't I get all the columns in the table? As * means all

    4. I used that method to check if the column existed, or if the result set was just empty. I was going to use rs.next(); however I thought that will go up one so I'll need to go back one, which would use more memory.

    5. Im kind of confused, I thought for selecting where I put 'WHEN' that would change it to p.getUniqueID();

    6. Ok I'll fix that!

    7. Ok I'll try to fix that as well!

    8. Hm alright, will try to figure that out.

    9. Ah my bad, fixed!

    10. Ye, my apologizes, I'll fix that.

    Thank you so much for helping me, hopefully I'll get the hang of it soon :D.
     
  4. I've fixed it up, does this look better?
    PHP:
    import com.connorlinfoot.bountifulapi.BountifulAPI;
    import org.bukkit.Bukkit;
    import org.bukkit.entity.Player;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.Listener;
    import org.bukkit.event.entity.PlayerDeathEvent;
    import org.bukkit.event.player.PlayerJoinEvent;
    import org.bukkit.event.player.PlayerQuitEvent;
    import org.bukkit.plugin.java.JavaPlugin;
    import org.bukkit.scheduler.BukkitRunnable;

    import java.sql.*;
    import java.util.HashMap;
    import java.util.UUID;

    public class Main extends JavaPlugin implements Listener {

        Connection conn = null;
        PreparedStatement statement;

        private HashMap<UUID, Integer> kills = new HashMap<UUID, Integer>();
        private HashMap<UUID, Integer> deaths = new HashMap<UUID, Integer>();
        private HashMap<UUID, Boolean> inDatabase = new HashMap<UUID, Boolean>();


        @Override
        public void onEnable() {

            getServer().getPluginManager().registerEvents(this, this);

            new BukkitRunnable() {

                @Override
                public void run() {
                    openConnection();
                }
            }.runTaskAsynchronously(this);

            Bukkit.getScheduler().scheduleSyncRepeatingTask(this, new Runnable() {
                @Override
                public void run() {
                    for (Player p : Bukkit.getOnlinePlayers()) {
                        updateActionBars(p);
                    }
                }
            }, 0, 20);
        }

        @Override
        public void onDisable() {
            for (UUID uuid : deaths.keySet()) {
                int pDeaths = deaths.get(uuid);
                int pKills = kills.get(uuid);
                String sql = "INSERT INTO stats(uuid,kills,deaths) "
                        + "VALUES(?,?,?)";
                try {
                    statement = conn.prepareStatement(sql);
                    statement.setString(1, uuid.toString());
                    statement.setInt(2, kills.get(uuid));
                    statement.setInt(3, deaths.get(uuid));
                    //Sends to database
                    statement.execute();
                    kills.remove(uuid);
                    deaths.remove(uuid);
                    statement.close();
                } catch (SQLException e1) {
                    e1.printStackTrace();
                }
            }
        }

        @EventHandler
        public void onLeave(PlayerQuitEvent e) {
            Player p = e.getPlayer();
            UUID uuid = p.getUniqueId();
            if (kills.containsKey(uuid) && deaths.containsKey(uuid) && inDatabase.containsKey(uuid)) {
                String sql;
                if (inDatabase.get(uuid)) {
                    sql = "INSERT INTO stats(uuid,kills,deaths) "
                            + "VALUES(?,?,?)";
                } else {
                    sql = "UPDATE stats(uuid,kills,deaths) "
                            + "VALUES(?,?,?)";
                }
                try {
                    statement = conn.prepareStatement(sql);
                    statement.setString(1, p.getUniqueId().toString());
                    statement.setInt(2, kills.get(uuid));
                    statement.setInt(3, deaths.get(uuid));
                    //Sends to database
                    statement.execute();
                    kills.remove(uuid);
                    deaths.remove(uuid);
                    statement.close();
                } catch (SQLException e1) {
                    e1.printStackTrace();
                }
            }
        }

        //Open MySQL Database Connection
        private synchronized void openConnection() {
            try {
                conn = DriverManager.getConnection("jdbc:mysql://localhost:3306/mysqljdbc", "username", "password");
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }

        @EventHandler
        public void onJoin(PlayerJoinEvent e) {
            Player p = e.getPlayer();
            UUID uuid = p.getUniqueId();
            String sql = "SELECT * FROM stats WHERE ID = " + p.getUniqueId().toString();
            try {
                ResultSet rs = conn.prepareStatement(sql).executeQuery();
                if (rs.isBeforeFirst()) {
                    //Column is in the database in the database
                    kills.put(uuid, rs.getInt("kills"));
                    deaths.put(uuid, rs.getInt("deaths"));
                    inDatabase.put(p.getUniqueId(), true);
                } else {
                    //Column is not in the database in the database
                    kills.put(uuid, 0);
                    deaths.put(uuid, 0);
                    inDatabase.put(p.getUniqueId(), false);
                }
                //Closing ResultSet
                rs.close();
            } catch (SQLException exception) {
                exception.printStackTrace();
            }
        }

        @EventHandler
        public void onDeath(PlayerDeathEvent e) {
            Player p = e.getEntity();
            if(p.getKiller() == null) return;
            Player killer = p.getKiller();
            kills.put(killer.getUniqueId(), kills.get(killer.getUniqueId()) + 1);
            deaths.put(p.getUniqueId(), deaths.get(p.getUniqueId()) + 1);
        }

        private void updateActionBars(Player p) {
            BountifulAPI.sendActionBar(p, "§aKills: §b" + kills.get(p.getUniqueId()) + " §d|| §aDeaths: §b" + deaths.get(p.getUniqueId()));
        }


    }



     
     
  5. sothatsit

    Patron

    In the join and leave events you do not run the tasks asynchronously and in the join event you do not use the prepared statements. Asynchronous means off the main thread. Because interacting with the database takes a bit of time, it is preferred to run all database interaction asynchronously to avoid stalling the server. The whole idea with prepared statements is that you let the statement handle the values for you, removing the risk of sql injection. Adding the uuid of the player directly to the query is wrong and you should use the method PreparedStatement.setString instead.

    It is not needed, and inefficient to store the PreparedStatement as a class variable. I'm not quite sure why you do this.

    Hope this helps clear things up.

    The *, all, symbol means to get all information from the table based on the condition you gave. So, it would find a row with the uuid of that player and return all information about the player. The reason it is preferred to specify the rows you want back is because of efficiency. Why get it to send you all the data if you only need some of it? Nevertheless, I have always used the * symbol because I have had issues with specifying the columns I need with which I'm guessing was older versions of MySQL.

    The syntax for the select statement is
    SELECT <what information you want> FROM <table> WHERE <condition>
    In your table ID is not a column, so it won't know what to return or when to return it. You have to use the name of the column you are searching over, which would be the uuid.

    A working statement would be
    SELECT * FROM stats WHERE uuid = ?;
    You would then replace the ? with the uuid using prepared statements to avoid sal injection.

    Another point is that statements should always end with a semi-colon ( ; ).
     
    • Informative Informative x 1