Optimization of plugin

Discussion in 'Spigot Plugin Development' started by TheWackyTV, Apr 21, 2017.

  1. Hi!

    I've coded a Plugin, and I can see that it's not very well optimized (timings) as you can see here
    [​IMG]

    Here's the code for that event:
    Code (Text):
    @EventHandler
        public void kill(PlayerDeathEvent e) {
            Player killer = e.getEntity().getKiller();
            LevelManager.killEvent(killer.getUniqueId().toString());
            BossBarAPI.removeAllBars(e.getEntity().getPlayer());
            int kills = LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString());
            if(kills == 10) {
                BossBarAPI.setMessage(killer, "§c§lLEVEL UP -> §e§l" + LevelManager.getLevel(killer.getUniqueId().toString()), 100f);
            } else if (kills == 9) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 10f);
            } else if (kills == 8) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 20f);
            } else if (kills == 7) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 30f);
            } else if (kills == 6) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 40f);
            } else if (kills == 5) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 50f);
            } else if (kills == 4) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 60f);
            } else if (kills == 3) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 70f);
            } else if (kills == 2) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckills for næste level", 80f);
            } else if (kills == 1) {
                BossBarAPI.setMessage(killer, "§cDu mangler §e" + LevelManager.getKillsTillNextLevel(killer.getUniqueId().toString()) + " §ckill for næste level", 90f);
            }
            new BukkitRunnable() {
                @Override
                public void run() {
                    BossBarAPI.removeAllBars(killer);
                    cancel();
                }
            }.runTaskLater(JavaPlugin.getPlugin(Core.class), 4 * 20);
        }

    Code (Text):
    public static void levelUP(String uuid) {
            int currentLevel = getLevel(uuid);
            int newLevel = currentLevel + 1;
            pl.getConfig().set("levels." + uuid + ".current-kills", 0);
            pl.getConfig().set("levels." + uuid + ".next-level", 10);
            pl.getConfig().set("levels." + uuid + ".current-level", newLevel);
            pl.saveConfig();
            Player p = Bukkit.getPlayer(UUID.fromString(uuid));
            if(p != null) {
                p.playSound(p.getLocation(), Sound.LEVEL_UP, 5, 5);
                p.sendMessage("");
                p.sendMessage("§8» §2§lNyt level");
                p.sendMessage("§8» §7Du er lige gået et level op, tillykke med det. Du er nu i lvl §e" + newLevel);
                p.sendMessage("§8» §aDu har modtaget §2150$ §afor at gå et level op.");
                p.sendMessage("§8» §dDu har modtaget §51 Rankup Key§d.");
                p.sendMessage("");
                Bukkit.getServer().dispatchCommand(Bukkit.getConsoleSender(), "eco give " + p.getName() + " 150");
                Bukkit.getServer().dispatchCommand(Bukkit.getConsoleSender(), "crate gk " + p.getName() + " rankup 1");
            }
        }

        public static void newKill(String uuid) {
            int currentKills = getKillsTillNextLevel(uuid);
            pl.getConfig().set("levels." + uuid + ".current-kills", currentKills + 1);
            pl.getConfig().set("levels." + uuid + ".next-level", currentKills - 1);
            pl.saveConfig();
        }

        public static void killEvent(String uuid) {
            int currentKills = getKillsTillNextLevel(uuid);
            if(currentKills == 1 || currentKills < 1) {
                levelUP(uuid);
            } else {
                newKill(uuid);
            }
        }

    I'd like to hear what I can do better?

    Thanks!
     
  2. save the new kills in a hashmap and set the value to all players in onDisable or in a runnable but not every time
     
    • Like Like x 1
  3. I'll try, thanks!:)
     
  4. Choco

    Moderator

    The killer can return null if a Player did not kill Player in the event. You should check for this before continuing to use it

    Any particular reason as to why you're not using UUID objects? If you're storing this information, UUID objects use about 1/5th of the memory than a String

    There's no need to call #getPlayer() on this one, because that will simply return OfflinePlayer. #getEntity() returns a Player object already (I know.. poor choice of method name).

    Wow... okay, this can be condensed a lot
    Code (Java):
    private static final int MAX_KILLS = 100;

    public void yourListenerMethod() {
        // All the code you had before what is quoted
        BossBarAPI.setMessage(killer, "§c§lLEVEL UP -> §e§l" + LevelManager.getLevel(killer.getUniqueId().toString()), kills == 10
            ? 100
            : MAX_KILLS - (kills * 10));
    }
    Also, you really should not be hard-coding the section symbol because some operating systems do not actually support it, and it results in strange question mark characters and white text. It's best to use the ChatColor API for this reason. (I didn't replace them in the above example because I'm rather lazy, but I know you can do it ;))

    Save this in a constant field rather than retrieving it every time a Player dies

    EDIT: I just realized that your LevelManager reads and writes to the Configuration File quite a lot. I recommend having a Set<UUID> (or a Map<UUID, Integer>... depends on what you're storing) to hold information locally rather than reading and writing constantly. That is likely what is causing a bit of overhead. Write locally, and periodically save to file (every 5, 10, 15 minutes or so) to prevent any loss of data.
     
    #4 Choco, Apr 21, 2017
    Last edited: Apr 21, 2017
    • Informative Informative x 1
  5. Its already said. Hashmaps are your Friends!!! Mayne FC the system for ram optimization!
     
  6. Plus for cleanliness replace that huge if statement with a switch.
     
  7. Choco

    Moderator

    I gave a 3 line alternative (It could be 1 line, but I like my code to be slightly organized). No need for a switch statement
     
  8. Ugh this have always bugged me! Even though the compiler tells me what object it returns, but it always takes a moment of realization for me after I look confused on the name every time.

    That was a bit off topic, on my phone rn but would love to provide some code for your optimization.

    Maybe create your own player object and keep track of the kills. Then use getKills() and compare it to how many kills are left.

    For example:
    int killsNeeded = 10;
    int myKills = MyPObject#getKills();
    "Kills left: " + (killsNeeded - myKills)

    You are after all writing in an object orientated language, how about taking an object orientated approach? Keeping track of player data such as kills isn't hard. If you want it persistent, you'd might have to serialize and such to a file but if it's simple you can just create a new MyPObject and using a Map<UUID, MyPObject> to keep track of it, then that's their object. If their uuid is not in the map, create a new MyPObject and add it to the map with their uuid. Simple and very elegant in case you need to store other player data primitive values as well such as deaths maybe
     
  9. Choco

    Moderator

    Not sure now necessary a whole player wrapper is if only kills are being tracked. If more data is being stored, then yes, a wrapper would be better suited. Otherwise, just a Map<UUID, Integer> will suffice
     
  10. I mean, I personally think the wrapper is always. neccecary. As it encourages an object orientated solution, and it would just be stupid to change later if let's say they wanted to store deaths and other data such as team.