Why isnt this for loop workingq

Discussion in 'Spigot Plugin Development' started by Kyllian, Apr 15, 2017.

  1. Hey

    This is my code:

    Code (Text):
    for (String str : cooldownCmds) {
                Bukkit.broadcastMessage(str);
                Bukkit.broadcastMessage(e.getMessage());
                if (e.getMessage().startsWith(str)) {
                    Bukkit.broadcastMessage("yes");
                    e.setCancelled(true);
                    continue;
                } else {
                    e.setCancelled(false);
                    Bukkit.broadcastMessage("no");
                    return;
                }
            }
            data.toExecute = e.getMessage();
            Bukkit.broadcastMessage(data.toExecute);
            Bukkit.broadcastMessage(e.getMessage());
            data.timeLeft.put(uuid, main.getConfig().getInt("Delay"));
    It's looping though every string in the config. When ONE of the x amount of string matches, the loop should stop. And continue with the code:

    Code (Text):
            data.toExecute = e.getMessage();
            Bukkit.broadcastMessage(data.toExecute);
            Bukkit.broadcastMessage(e.getMessage());
            data.timeLeft.put(uuid, main.getConfig().getInt("Delay"));
    And all of them doesnt match. It should do:

    Code (Text):
                    e.setCancelled(false);
     
  2. You're using the continue statement. Which means to continue the loop.

    Replace it with break and the loop will stop. Also no need to set the event cancelled to false as it defaults to false unless something else cancels it


    Sent from my iPhone using Tapatalk
     
  3. Alright thanks, That works.

    Do you know what this problem is:

    My timer doesn't start. Altought I'm sure it should

    Main
    Code (Text):
    package me.kyllian.minefly;

    import org.bukkit.Bukkit;
    import org.bukkit.plugin.java.JavaPlugin;
    import org.bukkit.scheduler.BukkitRunnable;

    import me.kyllian.minefly.listeners.OnPlayerCommandPreprocessEvent;
    import me.kyllian.minefly.utils.Data;

    public class MineFly extends JavaPlugin {

        private Data data = Data.getInstance();

        public static MineFly main;

        public static MineFly getInstance() {
            return main;
        }

        public void onEnable() {
            main = this;

            getConfig().options().copyDefaults(true);
            saveDefaultConfig();
           
            runnableRunner();

            Bukkit.getPluginManager().registerEvents(new OnPlayerCommandPreprocessEvent(), this);
        }

        public void runnableRunner() {
            new BukkitRunnable() {
                @Override
                public void run() {
                    if (data.timeLeft.isEmpty()) {
                        return;
                    }
                    for (String uuid : data.timeLeft.keySet()) {
                        int timeLeftOld = data.timeLeft.get(uuid);
                        data.timeLeft.remove(uuid);
                        data.timeLeft.put(uuid, timeLeftOld - 1);
                        Bukkit.broadcastMessage(data.timeLeft.get(uuid).toString());

                        if (data.timeLeft.get(uuid) == 0) {
                            data.timeLeft.remove(uuid);
                            // close things
                            // execute commands
                        }
                    }

                }
            }.runTaskTimer(this, 0, 20);
        }
    }
     
    Where I call it:

    Code (Text):
    package me.kyllian.minefly.listeners;

    import java.util.List;

    import org.bukkit.Bukkit;
    import org.bukkit.entity.Player;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.Listener;
    import org.bukkit.event.player.PlayerCommandPreprocessEvent;

    import me.kyllian.minefly.MineFly;
    import me.kyllian.minefly.utils.ColorTranslate;
    import me.kyllian.minefly.utils.Data;

    public class OnPlayerCommandPreprocessEvent implements Listener {
       
        private MineFly main = MineFly.getInstance();
        private ColorTranslate ct = new ColorTranslate();
       
        @EventHandler
        public void onPlayerCommandPreprocessEvent(PlayerCommandPreprocessEvent e) {
            e.setCancelled(true);
            Player p = e.getPlayer();
            if (p.hasPermission(main.getConfig().getString("OverridePermission"))) {
                e.setCancelled(false);
                return;
            }
            String uuid = p.getUniqueId().toString();
            Data data = Data.getData(p);
            if (data.toExecute != null || data.timeLeft.get(uuid) != null) {
                p.sendMessage(ct.cc(main.getConfig().getString("InCooldown")));
                return;
            }
            List<String> cooldownCmds = main.getConfig().getStringList("OnCommands");
            for (String str : cooldownCmds) {
                Bukkit.broadcastMessage(str);
                Bukkit.broadcastMessage(e.getMessage());
                if (!e.getMessage().startsWith(str)) {
                    Bukkit.broadcastMessage("no");
                    e.setCancelled(false);
                    continue;
                } else {
                    e.setCancelled(true);
                    Bukkit.broadcastMessage("yes");
                    break;
                }
            }
            data.toExecute = e.getMessage();
            Bukkit.broadcastMessage(data.toExecute);
            Bukkit.broadcastMessage(e.getMessage());
            data.timeLeft.put(uuid, main.getConfig().getInt("Delay"));
        }
    }
     
    Data class:

    Code (Text):
    package me.kyllian.minefly.utils;

    import java.util.HashMap;

    import org.bukkit.entity.Player;

    public class Data {

        public static HashMap<Player, Data> datas = new HashMap<Player, Data>();
        public HashMap<String, Integer> timeLeft = new HashMap<String, Integer>();

        private static Data dat = new Data();

        public static Data getInstance() {
            return dat;
        }

        public static Data getData(Player p) {
            if (p.isOnline()) {
                if (!datas.containsKey(p)) {
                    datas.put(p, new Data());
                }
            }
            return datas.get(p);
        }

        public String toExecute = null;
    }
     
     
    • Don't abuse static. If you're going to store an instance of Main and Data in a field, you might as well just pass it to the other class through it's constructor.
      • To make things more ironic, you're doing the exact reverse with ColorTranslate :p (the cc method should likely be static - I don't know it's code, but if you're just calling ChatColor::translateAlternateColorCodes, it's a stateless function (not relying on anything besides input parameters), which most certainly qualifies for static)
    • Use abstract types as field types, rather than implementation types (Use Map rather than HashMap)
    • Rather than
      Code (Java):
      if not contains
          put
      return get
      use Java 8's Map::computeIfAbsent(K, Function<K, V>)
    • Public fields are generally frowned upon, since anyone can set anything to the field (toExecute in Data). Make it private, and create a getter and setter.
      • To build upon this, returning mutable fields is an extremely bad idea, as it allows the receiver do do anything without the object knowing about it. Don't create getters/setters in this case, but create some methods to alter the state (in this case, create a method to set the time, accepting a Player and an int, which internally calls 'timeLeft.put(player, int)', etc.
    • For some reason, you create a static Data instance, but also track a Data instance per player. That does not seem to be correct to me (it's either one global one, or one per player, hardly ever - if even - both at the same time)
    • Before your for loop, create a boolean variable and set it to false. Whenever you find a command you want to apply a cooldown on, set it to true and break from the loop. Then, if the boolean is true, execute the last few lines. This should ensure it's only run when it needs to run (and not always, like it's currently the case)
    • Cancellable::setCancelled(false) is generally frowned upon, as it could lead to a lot of weird bugs with other plugins. You only want to cancel, or reset it to it's old state in almost every case.
      • If you want to skip events that are already cancelled, set ignoreCancelled to true in the EventHandler annotation
      • In your case, don't start with Cancellable::setCancelled(true). Only cancel it when aforementioned boolean is true (so in the block where you set their cooldown)
    • Using stringified UUIDs leads to less performance compared to using UUID objects (it takes less time to compare UUID objects than it takes to compare two Strings). It's advised to just use UUID objects instead (if you have them, of course, don't start creating UUIDs from random String objects now :p)
    • If you're iterating over a Map (whether it's it's key set, value collection, entry set), you can't directly remove entries from it (it'll throw a ConcurrentModificationException). Instead, use Iterators (which have this handy Iterator::remove() method which removes whatever was last returned by Iterator::next())
      • This applies to any kind of Collection, for future reference
    • Instead of looping over keys, and using Map::get(K) to get the value, iterate over the entry set instead (which gives you an Entry<K, V>, which already has both the key and value)
     
    • Informative Informative x 2
    • Winner Winner x 1