How to make your plugin better.

Discussion in 'Resources' started by Gravity, May 28, 2012.

Thread Status:
Not open for further replies.
  1. Offline

    Gravity Retired Staff

    There are a lot of tutorials out there on how to make a plugin and tons of different ways of doing it. However, there are almost no tutorials on improving the quality of your plugin. Seeing as I go through a fair number of other people's content every day, I figured that I am slightly qualified to determine what is bad coding that people use all the time.

    However, this tutorial will basically just be a bunch of tips. It's not all about coding, rather about how your plugin looks and feels to it's users. Your audience is THE most important part of making a plugin for the community, and don't you ever forget that; hopefully with a few tips you will be on your way to having happy users and more downloads. Note that this page is a work in progress, when I see something that is constantly used and is not good, I'll add the tip here. Be sure to keep checkin' back! Without further ado, let's get working.

    Tip #1: Stop excluding ConsoleCommandSender!
    This one annoys me to no end. I'm using a plugin, I really need to just do something from the console instead of logging in, and I see "Sorry, this command cannot be used from the console"... What? Why in the hell would "Kill" not be able to be used from the console? I understand things like /sethome and other things that actually require a player entity to work, but most plugin developers get lazy and just weed out anything that's not a player first. Here's how to fix this:

    Most people do something like this:
    Code:
    @Override
    public boolean onCommand(CommandSender cs, Command cmd, String alias, String[] args)
    {
      if(cs instanceof Player)
      {
          //Command magic goes here
      }
      else
      {
          cs.sendMessage("Sorry, but this command can only be used in-game");
      }
      return true;
    }
    Stop that! You will easily aggravate your users and when they need to get stuff done they will likely find your plugin annoying and go to find and alternative. Instead of using player.sendMessage() for your responses to the player, just use cs.sendMessage() all the time. If the CommandSender is a console, the message will be sent to the console. If the CommandSender is a player, the message will be sent to a player. It's fantastic. Same goes for cs.hasPermission(). Basically, if the command you are writing will work from the console and does not require a call like player.getLocation() or something, you can feel free to make sure it's a player first, and disallow consoles for obvious reasons. Otherwise, make it work from the console!

    Tip #2: Tell people what they are doing wrong!

    Nothing is worse than leaving someone in the dark when they are trying to use your plugin. Admins should only have to read documentation about the commands and permissions, and should from there be able to control the plugin. A rule of thumb to follow is that whenever a player issues a command, they should always get a response back. Always. If a player is doing something wrong in the command, tell them that and show them the right way to do it. If a player used the command in it's correct manner, let them know that the action has been taken.

    Let's look at a poor command:
    Code:
    @Override
    public boolean onCommand(CommandSender cs, Command cmd, String alias, String[] args)
    {
      if(cmd.equalsIgnoreCase("kill"))
      {
        if(args.length == 1)
        {
          Player player = getServer().matchPlayer(args[0]);
          if(player != null)
          {
            player.setHealth(0);
          }
        }
      }
      return true;
    }
    This is bad code because there are no else statements. If the command gets weeded out by one of these if statements, how is the CS supposed to know that? We should be telling them what goes wrong or what goes right. Let's improve this code:
    Code:
    @Override
    public boolean onCommand(CommandSender cs, Command cmd, String alias, String[] args)
    {
      if(cmd.equalsIgnoreCase("kill"))
      {
        if(args.length == 1)
        {
          Player player = getServer().matchPlayer(args[0]);
          if(player != null)
          {
            player.setHealth(0);
            cs.sendMessage("Killed "+args[0]);
          }
          else
          {
            cs.sendMessage("Could not find a player by the name "+args[0]);
          }
        }
        else
        {
          cs.sendMessage("Usage: /kill [player]");
        }
      }
      return true;
    }
    Much better! Now we know exactly what's going on, and there is no point where a player would issue the /kill command and not know if they did something right or wrong.

    Tip #3: Stop spamming console!

    Many people print messages out when their plugin is enabled, attempting to let the admin know what is going on when the plugin loads. Unfortunately, most admins don't need to know this information and don't care about it as long as the plugin is working, so you are effectively just spamming them, and if your plugin outputs a lot of information onEnable(), you will likely loose a lot of users. When Bukkit enables a plugin, it actually prints out that the plugin has been enabled, so there's no need for you to print out anything at all until later in the plugin if you want to tell the console something.

    TnT has the scoop on the spamming issue, he has a request to all plugin developers that they use a "verbose logging" configuration option in their plugin, so that admins only see this information when it is needed. I encourage you to go read and apply his suggestion to your plugins.

    Tip #4: Get your own logger!
    Did you know that Bukkit gives each plugin their own logger to use for alerting the console?

    Doing what most people do..
    Code:
    Logger logger = Logger.getLogger("Minecraft");
    Will force you to prefix every message you send so that server admins know where it's coming from. Instead of doing that, just use the logger provided to you like so:
    Code:
    this.getLogger().log(Level.INFO,"Hello World!");
    Bukkit will automatically prefix your message with your plugin's name, and admins will never again have to find what is sending that message.

    Tip #5: Be careful with static references!
    Java constants like
    Code:
    private static final int DEFAULT_VALUE = 1;
    are ok, as long as you stick to primitive data types and Strings. Static references to collections and other structures (like your plugin class) however can cause memory leaks if you are to lazy to unset (assign null) them.

    This problem is caused by the way the PluginClassLoader works: When the server reloads, a new PluginClassLoader will be instantiated. Static members (or the references they maintain) are only valid in the same class loader that also loaded your plugin class. If the server reloads, the static references are still assigned and used by the "old" plugin class instance, which effectively prevents the garbage collector from cleaning up the memory. This the most common reason for memory leaks caused by plugins.

    If you are (for what reason ever) dependent on static references, assign null to them when your plugin gets disabled! You can also usually avoid storing things like player objects that could cause memory leaks by instead storing the player's name, and then later you can get the player with that name.

    This static member doesn't have to be nulled because it is a primitive value (which can't be nulled at all):
    Code:
    private static final int DEFAULT_VALUE = 1;
    These static members must be nulled in order to prevent memory leaks, because they are structures:
    Code:
    private static Set<AnyClass> mySet = new HashSet<>();
    private static MyPlugin plugin;
    Another tip:
    Imagine your plugin as a mobile:
    Show Spoiler
    [​IMG]

    The PluginClassLoader is the roof it is hanging on.
    It has one element supporting all other elements: the string, which is your plugin class. All elements are (not necessarily directly) connected through the plugin class. Therefore (you may already imagine it) the plugin class should be the only class which is accessible through a static reference. All other references should be accessible through non-static getters.

    Example skeleton:
    Code:
    public class MyPlugin extends JavaPlugin {
     
      private static MyPlugin instance = null;
     
      private MyListener myListener;
     
      public MyListener getMyListener() {
        return myListener;
      }
     
      @Override
      public void onEnable() {
        instance = this;
        myListener = new MyListener();
        Bukkit.getPluginManager().registerEvents(myListener, this);
      }
     
      @Override
      public void onDisable() {
        instance = null;
      }
     
    }
    
    And that's all I've got for now.. If you have any other suggestions feel free to leave them. There's a lot more tips and I'll be adding onto this regularly, but for now that's all I can think of :)

    Thanks to: obnoxint
    for helping out :)
     
  2. Offline

    Wundark

    In Tip #4 you made a mistake.
    Code:
    this.getLogger.log(Level.INFO,"Hello World!");
    It should be (Missed the () )
    Code:
    this.getLogger().log(Level.INFO,"Hello World!");
    Or
    Code:
    this.getLogger().info("Hello World!");
     
  3. Offline

    Gravity Retired Staff

    Fixed, thanks
     
  4. Offline

    Njol

    Shouldn't that be a simple "return false;" to display the usage as defined in the plugin.yml? (also the method is missing a return statement at the end ;))
    I haven't run a server for a long time so I can't tell whether these are common issues, but these are issues no plugin should have.
     
  5. Offline

    Gravity Retired Staff

    I typically just return true no matter what and send my own usage messages, that way any message you get back from the plugin is uniform and I can customize it. Just personal preference, though.
     
    np98765 and Codex Arcanum like this.
  6. Offline

    tips48

    Two things that kill me, one more important than the other:
    • Tons of public (and/or static) variables in the main plugin class. I"ve seen way to many errors to consider this more effective than taking the extra time to create getters and setters
    • A getInstance() method..It's not that hard to pass an instance of your plugin, and this can create alot of errors when your plugin is reloaded
     
  7. Offline

    kumpelblase2

    I do agree with the first one. However, I honestly disagree with the second one. The reason is that for example in my plugin I'd need to give kinda every class a reference to my main class to work properly. If I wouldn't have the opportunity to reference the instance of my main class in a static way I would sometimes not be able to do certain things, which are needed, because in the rare classes which don't need that instance of my main directly I would need it to create the other classes which need the main class. Generally that would cause more problems than it might solve. If you handle static values properly, a static getInstance() is often a lot easier and better than passing a reference to your main all the time. Though, if you have a small plugin, a static main instance or getInstance() method is not needed, which is correct. But you can't generalize that.
     
  8. Offline

    tips48

    Yea, the second one is much less important. I only say that because it's much harder to write test cases for, and I like having everything testable :)
     
  9. Offline

    Njol

    I use a static getInstance method in my plugin, and even tested not allowing for another instance of the main class to be created (by throwing an exception in the constructor if instance != null), reloaded the server and got no errors at all. It seems as if Bukkit completely reloads the classes when reloading a plugin.
     
  10. Offline

    ferrybig

    at the commands, you never returned something, so they are both wrong
     
  11. Offline

    Gravity Retired Staff

    It's just pseudo code really, it's not meant to be totally correct, but I'll fix it anyway...
     
  12. Offline

    tips48

    Testing = Unit tests :)
     
  13. Offline

    ZachBora

    Is the plugin logger static?

    I usually do something like MyPlugin.staticlogger.info()
     
  14. Offline

    tips48

    That means your defining your own..The new plugin logger uses MyPluginInstance.getLogger()
     
  15. Offline

    ZachBora

    That's my question, is MyPluginInstance.getLogger() static?
     
  16. Offline

    tips48

    Nope
     
  17. Offline

    ZachBora

    Could I do this in the plugin class?

    PHP:
    public static void insertLog(String text)
    {
      
    this.getLogger().info(text);
    }
     
  18. Offline

    kumpelblase2

    No since 'this' doesn't exist in a static context.
     
  19. Offline

    ZachBora

    so it's a drawback if I want to use the plugin's logger.
     
  20. Offline

    tips48

    Sure, I guess. Just include an instance to your plugin, shouldn't be that hard.
     
  21. Offline

    ZachBora

    I changed the code to remove every instances, because I didn't need them.
    I'm used to coding that allows me to log or throw errors from any class. And I don't want to have just 1 class in my plugin. I never liked the instanciating the plugin just for to do logs approach.
     
  22. Offline

    blackwolf12333

    Well, i like the idea of giving every plugin it's own Logger, but it doesn't work :( it returns null when i try to use it...
    Btw, should i report this on https://bukkit.atlassian.net/secure/Dashboard.jspa ?
    greetz blackwolf12333
     
  23. Offline

    tips48

    It works fine...You can only use it after onLoad has been called.
    Yea, I agree it could be better...Eh :S
     
  24. Offline

    blackwolf12333

    Hmm ok, i'll try that.
    Thanks, that worked:)
     
  25. Offline

    Sagacious_Zed Bukkit Docs

  26. Offline

    Gravity Retired Staff

  27. Offline

    heisan213

    I always set the usage in plugin.yml then when I register commands, I set the usage message to CharColor.RED + cmd.getUsage().
    Red always makes things cooler ;)
     
  28. Offline

    obnoxint

    I would like to contribute this:

    Tip #5: Be careful with static references!
    Java constants like
    Code:
    private static int DEFAULT_VALUE = 1;
    are ok, as long as you stick to primitive data types and Strings. Static references to collections and other structures (like your plugin class) however can cause memory leaks if you are to lazy to unset (assign null) them.

    This problem is caused by the way the PluginClassLoader works: When the server reloads, a new PluginClassLoader will be instantiated. Static members (or the references they maintain) are only valid in the same class loader that also loaded your plugin class. If the server reloads, the static references are still assigned and used by the "old" plugin class instance, which effectively prevents the garbage collector from cleaning up the memory. This the most common reason for memory leaks caused by plugins.

    If you are (for what reason ever) dependent on static references, assign null to them when your plugin gets disabled!

    This static member doesn't have to be nulled because it is a primitive value (which can't be nulled at all):
    Code:
    private static int DEFAULT_VALUE = 1;
    These static members must be nulled in order to prevent memory leaks, because they are structures:
    Code:
    private static Set<AnyClass> mySet = new HashSet<>();
    private static MyPlugin plugin;
    Another tip:
    Imagine your plugin as a mobile:
    Show Spoiler
    [​IMG]

    The PluginClassLoader is the roof it is hanging on.
    It has one element supporting all other elements: the string, which is your plugin class. All elements are (not necessarily directly) connected through the plugin class. Therefore (you may already imagine it) the plugin class should be the only class which is accessible through a static reference. All other references should be accessible through non-static getters.

    Example skeleton:
    Code:
    public class MyPlugin extends JavaPlugin {
     
      private static MyPlugin instance = null;
     
      private MyListener myListener;
     
      public MyListener getMyListener() {
        return myListener;
      }
     
      @Override
      public void onEnable() {
        instance = this;
        myListener = new MyListener();
        Bukkit.getPluginManager().registerEvents(myListener, this);
      }
     
      @Override
      public void onDisable() {
        instance = null;
      }
     
    }
    
    If you have any questions or find something to be confusing, just ask!
     
    Europia79 and Icyene like this.
  29. Offline

    Gravity Retired Staff

    Sweet! Added.

    EDIT: In your example you should also null the listener, correct?
     
  30. Offline

    Njol

    Would it be too difficult for Bukkit NOT to reload all classes >_>? I like static and I'm not gonna change my whole plugin now...
    But if this is really an issue I will simply write a small function to set all static variables to null using reflection in onDisable().

    Edit: what about final static variables?
     
Thread Status:
Not open for further replies.

Share This Page