Solved Cooldown Command Issue

Please understand my JAVA is awful so if you are going to suggest something please give me an example with the code you give for example; Saying use method this and that I won't get it I am a total fool!

As of right now my cooldown command which I am trying to setup works 90% Until the cooldown is over in which it will not allow me to recast the command?
This is how it goes :
/Hunger
Nom Nom
/Hunger - You have to wait another 5 seconds to use /hunger again!
5
4
3
2
1
/Hunger - Nothing happens, Command registers in console but nothing happens in game.


This is my code :

Code:

package com.avairu.basic;

import java.util.HashMap;

import org.bukkit.ChatColor;
import org.bukkit.command.Command;
import org.bukkit.command.CommandExecutor;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;

public class HungerC implements CommandExecutor {
        HashMap<String, Long> hunger = new HashMap<String, Long>();
        @SuppressWarnings("unused")
        private Avairu cooldown;
        public HungerC(Avairu cooldown) {
        this.cooldown = cooldown;
        }

        public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args){
            if(cmd.getName().equalsIgnoreCase("hunger")){
            int cooldown = 5;
            Player s = (Player)sender;
            if(sender instanceof Player){
               if(args.length > 0){
                   sender.sendMessage(ChatColor.RED + "Too many arguments!");
                   return false;
               }
               if (hunger.containsKey(s.getName())) {
                  long diff = (System.currentTimeMillis() - hunger.get(s.getName()))/1000;
                      if (diff < cooldown) {
                           s.sendMessage(ChatColor.RED + "You have to wait another "+(cooldown - diff)+" seconds to use /hunger again!");
                       }
               } else {
                  s.setFoodLevel(20);
                  s.setSaturation(10);
                  s.sendMessage(ChatColor.RED + "You scoff down a pie you found in your back pocket!");
                  hunger.put(s.getName(), System.currentTimeMillis());
               }
            }else {
                    System.out.println("You must be a player to use /hunger!");
                    sender.sendMessage(ChatColor.RED + "You must be a player to use /hunger!");
                    return false;
            }
            return true;
            }
            return false;
        }
}

 

@JBAvodas Maybe your Java is awful, but we can't really read it that way.
Please put [ code=java ] and [ /code ] tags around it (without spaces of course) and don't forget to indent it.

EDIT: After trying to read it like this, I found your problem:

Code:

if (hunger.containsKey(s.getName())) {
    long diff = (System.currentTimeMillis() - hunger.get(s.getName()))/1000;
    if (diff < cooldown) {
        s.sendMessage(ChatColor.RED + "You have to wait another "+(cooldown - diff)+" seconds to use /hunger again!");
    }
} else {
    s.setFoodLevel(20);
    s.setSaturation(10);
    s.sendMessage(ChatColor.RED + "You scoff down a pie you found in your back pocket!");
    hunger.put(s.getName(), System.currentTimeMillis());
}

Remove the else part, and add one to the "diff < cooldown" if, where you remove the player from the hashmap.

 

Sorry for the code error, I don't understand this website just yet sorry!

So what your saying is to make it instead of else change it to

if (diff < 1) { ???

 

@JBAvodas No, do it like that:

Code:

if (something) {
    if (something_else) {
        // Code here
    } else {
        // Code here
    }
}
// Code here

 

So does this make more sense?

Code:

public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args){
            if(cmd.getName().equalsIgnoreCase("hunger")){
            int cooldown = 5;
            Player s = (Player)sender;
            long diff = (System.currentTimeMillis() - hunger.get(s.getName()))/1000;
            if(sender instanceof Player){
                if(args.length > 0){
                    sender.sendMessage(ChatColor.RED + "Too many arguments!");
                    return false;
                }
                if (hunger.containsKey(s.getName())) {
                        if (diff < cooldown) {
                            s.sendMessage(ChatColor.RED + "You have to wait another "+(cooldown - diff)+" seconds to use /hunger again!");
                        }else{
                            s.setFoodLevel(20);
                            s.setSaturation(10);
                            s.sendMessage(ChatColor.RED + "You scoff down a pie you found in your back pocket!");
                            hunger.put(s.getName(), System.currentTimeMillis());
                        }
                    }
                }
            }
            return false;
        }
    }

 

Maybe you could try this:

Code:

HashMap<String, Integer> hunger = new HashMap<String, Integer>();
    
        public HungerC() {}
        public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args){
            if(cmd.getName().equalsIgnoreCase("hunger")){
                if (!(sender instanceof Player)) {
                    sender.sendMessage(ChatColor.RED + "You must be a player to use /hunger!");
                    return false;
                }
            
                Player s = (Player) sender;
            
                if (args.length > 0) {
                    s.sendMessage(ChatColor.RED + "Too many arguments!");
                    return false;
                }
            
                if (hunger.containsKey(s.getName())) {
                    s.sendMessage(ChatColor.RED + "You have to wait another " + hunger.get(s.getName()) + " seconds to use /hunger again");
                    return false;
                }
            
                s.setFoodLevel(20);
                s.setSaturation(10);
                s.sendMessage(ChatColor.RED + "You scoff down a pie you found in your back pocket!");
                hunger.put(s.getName(), 5);
                return true;
            }
            return false;
        }

        @SuppressWarnings("deprecation")
        @Override
        public void run() {
            for (String player : hunger.keySet()) {
                if (Bukkit.getPlayer(player) != null) {
                    if (hunger.get(player) != 0) {
                        hunger.put(player, hunger.get(player) - 1);
                    } else {
                        hunger.remove(player);
                    }
                } else {
                    hunger.remove(player);
                }
            }
        }

Put

Code:

Bukkit.getScheduler().runTaskTimer(this, new HungerC(), 20L, 20L);

in your main class in your onEnable();

 

@stefvanschie

1. Spooonfeeed
2. Some parts of your code aren't good
   e.g. "public HungerC(){}"
3. Your method is poorly optimized, the original code is way faster
4. His code will work fine if he changes what I said.

 

@au2001
1. He said he was bad at Java and wanted to show him the code
2. Because he made it not needed unless he doesn't want to call, forgot to remove.
3. You think calling System.getCurrentMillis() is better?!
4. Yes, but still the System.getCurrentMillis() is bothering me.

 

@stefvanschie Yes, calling System.getCurrentMillis() once in a while is way better than calling a method every second.

 

@au2001 Calling a method once a second, shouldn't be a problem. If it would be he probably couldn't run the plugin. And because he's new to java (and I assume to Bukkit aswell) it's good to learn how schedulers work.

 

Well I suppose if you're creating a cooldown system using commands, you can extend off of this code I made (tested):

UPDATED: Should Work

Code:java

 
package com.mrgreen33gamer.test;
 
import java.util.HashMap;
import java.util.Map;
 
import org.bukkit.ChatColor;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
import org.bukkit.plugin.java.JavaPlugin;
import org.bukkit.scheduler.BukkitRunnable;
 
public class Main extends JavaPlugin {
 
    public Main plugin;
 
    @Override
    public void onEnable(){
        plugin = this;
        cooldownLoop();
    }
 
    public HashMap<String, Integer> cooldown = new HashMap<String, Integer>();
 
    public void cooldownLoop(){
        new BukkitRunnable(){
            @Override
            public void run() {
                if(cooldown.isEmpty()) return;
                for (Map.Entry<String, Integer> entry : cooldown.entrySet()) {
                   String keyValues = entry.getKey();
                   if(cooldown.get(keyValues) <= 0){
                      cooldown.remove(keyValues);
                   }
                   cooldown.put(keyValues, cooldown.get(keyValues) - 1);
                }
            }
        }.runTaskTimer(this, 0, 20);
    }
 
    public boolean onCommand(CommandSender sender, Command cmd, String lbl, String[] args){
        if(lbl.equalsIgnoreCase("test")){
            if(sender instanceof Player){
                Player player = (Player) sender;
                if(!cooldown.containsKey(player.getName())){
                    cooldown.put(player.getName(), 10);
                    player.sendMessage(ChatColor.GREEN + "You've used this command! You're on a 1 minute cooldown!");
                }else{
                    player.sendMessage(ChatColor.RED + "You're on a cooldown of: " + cooldown.get(player.getName()));
                }
            }
        }
        return false;
    }
 
}
 
 
[syntax][/syntax]

 

@mrgreen33gamer Saving a long (unix time) would probably cause less lag and be clearer.

 

@au2001 As long as HashMaps are not static, it shouldn't make a 1ms difference, and using HashMaps are more efficient then checking for System milliseconds.

EDIT:

On top of that, it makes your classes a little more less clustered.

 

Thanks guys! Got it fixed now just gotta work out this forum and how to change this one to solved xD

 

@mrgreen33gamer "a little more less" that says it all
I wasn't talking about HashMaps (since my method uses one too) but the Runnable.