Critique a new dev's code

Basically, I have a general knowledge on Java programming, yet I have not much experience in Bukkit besides what I was able to scrounge up from the wiki and tutorial threads on the this and the Spigot forums. I figured that posting a short plugin of my work on here would help me find out what is irreverent, if there is anything that I should include in my code, anything should be formatted differently to look neater/more conventional. What I was going for with the plugin was just a timer that would countdown from 30 then display a message.

(pastebin http://pastebin.com/d1QARgaD since it looks nicer than the code block)

Code:

package io.github.penguinhi5.timer;
import org.bukkit.Bukkit;
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.scoreboard.DisplaySlot;
import org.bukkit.scoreboard.Objective;
import org.bukkit.scoreboard.Score;
import org.bukkit.scoreboard.Scoreboard;
import org.bukkit.scoreboard.ScoreboardManager;
public final class Timer extends JavaPlugin {
    public void onEnable() {
        getLogger().info("onEnable has been invoked!");
    }
 
    public void onDisable() {
        getLogger().info("onDisable has been invoked!");
    }
    public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args) {
        if (cmd.getName().equalsIgnoreCase("timer")) {
            Bukkit.getScheduler().runTaskTimer(this, new Runnable() {
                int seconds = 30;
             
                public void run() {
                ScoreboardManager manager = Bukkit.getScoreboardManager();
                Scoreboard timer = manager.getNewScoreboard();
                Objective objective = timer.registerNewObjective("timer", "dummy");
                objective.setDisplaySlot(DisplaySlot.SIDEBAR);
                objective.setDisplayName(ChatColor.GREEN + "" + ChatColor.BOLD + "Timer");
                Score space = objective.getScore("");
                space.setScore(2);
                Score time = objective.getScore(ChatColor.GREEN + "" + ChatColor.BOLD + "Starting in " + ChatColor.BLUE + seconds + ChatColor.GREEN + " Seconds");
                time.setScore(1);
             
                if (seconds == 0){
                    Bukkit.getServer().getScheduler().cancelAllTasks();
                    Bukkit.broadcastMessage(ChatColor.RED + "" + ChatColor.BOLD + "The timer has ended!");
                } else {
                    seconds--;
                }
             
                for (Player online : Bukkit.getOnlinePlayers()) {
                    online.setScoreboard(timer);
                }
                }
            }, 0L, 20L);
         
            return true;
        }
        return false;
    }
}

 

First of all, you're missing a few @Override annotations, as they are overriding the methods in the JavaPlugin superclass.

Other than that, the only concern would be the use of blank lines. It's really based on your personal preference but this is how I would do it:

Code:

public final class Timer extends JavaPlugin {
    @Override
    public void onEnable() {
        getLogger().info("onEnable has been invoked!");
    }

    @Override
    public void onDisable() {
        getLogger().info("onDisable has been invoked!");
    }

    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args) {
        if (cmd.getName().equalsIgnoreCase("timer")) {
            Bukkit.getScheduler().runTaskTimer(this, new Runnable() {
                int seconds = 0;

                @Override
                public void run() {
                    ScoreboardManager manager = Bukkit.getScoreboardManager();
                    Scoreboard timer = manager.getNewScoreboard();

                    Objective objective = timer.registerNewObjective("timer", "dummy");
                    objective.setDisplaySlot(DisplaySlot.SIDEBAR);
                    objective.setDisplayName(ChatColor.GREEN + "" + ChatColor.BOLD + "Timer");

                    Score space = objective.getScore("");
                    space.setScore(2);

                    Score time = objective.getScore(ChatColor.GREEN + "" + ChatColor.BOLD + "Starting in " + ChatColor.BLUE + seconds + ChatColor.GREEN + " Seconds");
                    time.setScore(1);

                    if (seconds == 0){
                        Bukkit.getServer().getScheduler().cancelAllTasks();
                        Bukkit.broadcastMessage(ChatColor.RED + "" + ChatColor.BOLD + "The timer has ended!");
                    } else {
                        seconds--;
                    }

                    for (Player online : Bukkit.getOnlinePlayers()) {
                        online.setScoreboard(timer);
                    }
                }
            }, 0L, 20L);

            return true;
        }

        return false;
    }
}

 

Thanks for the feedback! I'll add the @Override annotations and the blank spaces to make it look neater.

 

@PenguinHi5
Don't pint enable and disable messages, they are not needed since bukkit already does the for you.
Other than this it's pretty fine! Good job!

 

Bukkit.getScheduler().runTaskTimer(this, newRunnable(){ aint that depreciated?

 

No, Bukkit.getScheduler().runTaskTimer(this, new BukkitRunnable(){ is

 

You just posted the same thing.

 

BukkitRunnable is deprecated, not Runnable

 

Ah okay

 

@PenguinHi5 I say it's a good start. It's a good thing you know Java. Now the only thing you need to learn is the Bukkit API!

I may have to look at the JavaDocs again but I'm sure that the #registerNewObjective() method will throw an error if the objective is already registered.

I wouldn't quite do that. It might screw with other tasks. You can cancel a single task and using a BukkitRunnable makes things really easy to deal with. I suggest you check it out.

I'm pretty sure you only need to do that once. You don't need to set the player's scoreboard each time the task runs. There are some things in the runnable that you don't need because they only need to be ran once. The registering of your objective is another one. Call the code outside of the runnable instead.

 

Doesn't look deprecated to me:



You mean that some of the methods in BukkitScheduler that have ambiguous names are deprecated, which is the case.

In general, you should be using the BukkitRunnable instead of Runnable.

 

@mythbusterma

He didn't notice that mine said BukkitRunnable instead of Runnable, I didn't mean the class BukkitRunnable was deprecated, I ment the method BukkitScheduler#runTaskTimer(Plugin, BukkitRunnable, Long) is. Looking at it now however I could've worded it better

 

@PenguinHi5

You should have a class that handles the timer command. Most people do this by implementing CommandExecutor and overriding onCommand(). But I recommend some kind of command framework like sk89q's Intake or Alkarin's CustomCommandExecutor

Once you have your TimerExecutor, then in your onEnable() you'll do

Code:java

public TimerPlugin extends JavaPlugin {
 
    @Override
    public void onEnable() {
        getCommand("timer").setExecutor(new TimerExecutor(this));
    }
 
}

In general, it's just better to split your project up into many classes, each with a single responsibility.