Common Mistakes

Discussion in 'Plugin Development' started by mbaxter, Sep 12, 2012.

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

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    Common mistakes while coding a Bukkit plugin

    In this article I hope to cover a bunch of mistakes that I see frequently either in the Bukkit Plugin Development forum, the #bukkitdev IRC channel, or in plugins that I help folks write in other places on the net. All of these issues or mistakes can occur on a plugin that compiles fine.

    Building with Java 7 or Java 8
    While technically not a problem for your own computer, you need to keep in mind that plenty of Bukkit servers are running Java 6. You need to compile with Java 6 in order for them to use it (Java 7/8 users can use something built on 6). Minecraft, Bukkit and CraftBukkit are built for Java 6. Go grab Java JDK 6 and tell whatever you use to compile to use that instead!

    Denying Console from using commands that it could otherwise use
    I see a lot of plugins that, for commands not affecting the sender, deny anyone that isn’t a Player from using them. If your command can operate with console using it, why not allow it? Checking for instanceof Player prevents execution from console as well as other situations such as IRC plugins executing commands remotely.

    Not registering events or commands
    You’ve created your amazing event listener full of methods for catching all sorts of events. But none of them are firing! You created an onCommand method or a couple CommandExecutors for your commands but they aren’t triggering! Looks like you forgot to register your events and commands. Event registration is documented here, while command registration is documented here.

    Putting other plugins classes (or entire plugins) in your jar
    I’ve seen several developers, in the hope of ensuring their users also get the plugin theirs uses, include plugins or pieces of plugins in their code. First, this is often a license violation. More importantly this can breakthat other plugin and is a bad practice. Only include your own code (or code used with permission under license) in your plugins.

    Including CraftBukkit in your plugin
    This is certainly an impressive feat, but I’ve seen it in probably half a dozen submissions. Don’t include CraftBukkit (or Bukkit) in your plugin!

    Importing individual permissions or chat plugins for chat formatting (prefix/suffix)
    You create your awesome chat plugin that needs to get prefix/suffix data from other plugins. You release it and it works fine for 5 months. In those 5 months a brand new awesome prefix plugin comes out and you have to now edit in support, or one of the plugins you supported completely changes its internals. Your plugin needs constant updates to keep track of all the other plugins you’re checking just for some simple data they all provide. The plugin Vault was created precisely to avoid this scenario. Vault handles keeping track of all these systems for you and is an easy dependency to have your users install. They just drop it in and go. Done! On your end, you now have a single plugin to support whose API isn’t going to change.

    Using Vault to check if a Player has a permission
    On the opposite side of the Vault issue, is people who believe that using Vault to check a permission is the ideal way to go. I’ve seen plugins require Vault just for checking has(Player,permission). Fun fact: Vault’s permission check just calls Bukkit’s methods. There is zero benefit in using Vault to check a permission. Just call player.hasPermission(permission) and your life is far easier!

    Using non-threadsafe methods in Async events (AsyncPlayerPreloginEvent, AsyncPlayerChatEvent) or Async tasks (scheduler)
    Async events are not running as part of the minecraft thread. A good way of judging if you should be calling a method from an async event is asking yourself “does this method change or query something in-game?” If the answer is yes, you probably shouldn’t be doing it. Permission checks are not safe, as are many other methods. Pretty much, if you’re not certain, don’t.
    Examples of valid ways to deal with various async features:
    AsyncChat: Format the chat with your formatter. Use cached information about the player to format the chat.
    AsyncPrelogin: Query an external database (async is perfect for this, the delay in reply won’t lag the server) to determine if that username or IP can join.
    Async task: Update a MySQL database with new data.

    IO operations (writing non-config files, MySQL queries, metrics, update checks, etc) on the main thread
    IO calls can take a long time. Running your MySQL query or update check from the main thread can very quickly slow down the server in a highly noticeable way. Instead, schedule these actions to run in an async task. Need to do something that isn't thread-safe (see above) as a result of an IO operation? Schedule a sync task from that async task when it's time to react!

    Utilizing static variables for information sharing
    There are many reasons this is generally a poor decision. Here is a great description of some reasons: http://stackoverflow.com/questions/7026507/why-are-static-variables-considered-evil/7084473#7084473
    Storing constants statically is great. Storing temporary information is less so.

    Storing Player objects
    Store their UUID instead. Solve about 50 problems this way.

    You are welcome to comment below and even suggest additions.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 28, 2016
  2. Offline

    zack6849

    Im not sure if this is noteworthy, but i see newer developers that often forget to register their events and add the @EventHandler annotation.
     
  3. Offline

    ZeusAllMighty11 Retired Staff

    Forgetting to register commands in the plugin.yml ... may sound silly, but you'd be surprised
     
  4. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    MightyOne likes this.
  5. Offline

    zack6849

    That was there when i posted? or am i going insane?
     
  6. Offline

    desht

    Re: Denying Console from using commands that it could otherwise use - this is discussed in the General Plugin Tutorial at http://wiki.bukkit.org/Plugin_Tutorial#Console_Commands_vs._Player_Commands - might be worth adding a link to that mbaxter ?

    I'm a bit ambivalent about this one. What sort of problems, exactly? The one that springs to mind is that if a player logs off, you have a potential memory leak and hanging references, so you need to listen for the PlayerQuitEvent and perform any necessary cleanup.

    Storing the player name, on the other hand, is less prone to memory leakage etc., but when you need a Player object from the stored name (as you surely will sooner or later), you have to call getServer().getPlayer() or getServer().getPlayerExact(). These are slow methods, especially for busy servers. One might naïvely believe it's a just doing a HashMap lookup, but no: both methods do an array copy, followed by a linear search and compare of all logged-in players. Calling either method from e.g. a PlayerMoveEvent handler on a busy server is asking for lag.

    https://github.com/Bukkit/CraftBukk.../org/bukkit/craftbukkit/CraftServer.java#L306

    So I'm really not convinced anymore that storing player names all the time is such a good idea...

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 28, 2016
  7. Offline

    jacklin213

    man i just forgot that thanks

    typing the main class wrong in the plugin.yml, exporting with errors

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 28, 2016
  8. Offline

    Woobie

    Thats what has been happening to me all the time =(
     
  9. Offline

    jacklin213

    Lol
     
  10. Offline

    Sleaker

    While I agree with not using Vault for simple perms checks, doesn't the JVM optimize out any of the performance difference?

    And there is sometimes a reason to use Vault for perms checks: wanting to check perms on a world for a player that is on a separate world (if the perm plugin enabled supports it). And when needing to do offline permission checks, but both of these are few and far between.
     
  11. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    Sleaker I'm referring purely to plugins that require Vault in order to do perm checks, when they're only calling the has check that I know in the Vault code is just hasPermission. As for performance loss I'm more referring to handling Vault when it's unnecessary rather than the direct call. Added startup bloat, or in the case of many plugins, extra bloat everywhere because so many plugins seem to think it's necessary to re-find vault's services every call.
     
  12. Offline

    chaseoes Retired Staff

    Doesn't Vault support P3/older permissions plugins?
     
  13. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    stuntguy3000 likes this.
  14. Offline

    jacklin213

    er can someone go over if we should use @override or not cause ive watched a load of youtube tutorials, some people use it some people don't. This might confuse a lot of people (i dont cause im too lazy to type @override)
     
  15. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    Not a 'common mistake' or something that can cause issues. Use @Override over your methods that you believe are overriding a parent because if that status changes (parent loses that method) then your IDE will show an error that it's not overriding anything.
     
  16. Offline

    jacklin213

    how bout creating a public void and forgetting to call it. eg. public void configgen()
    and not putting configgen() into onEnable()
     
  17. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    for the most part I'm dealing with Bukkit-specific stuff. Forgetting to call a method you need is far more basic :3
     
  18. Offline

    Digi

    People keep writing enabled and disabled messages :\ it's pointless and can slow down startup and shut down... and for what ?!
     
  19. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    How does sending a message slow down anything significantly?
     
  20. Offline

    Acrobot

    It doesn't slow down startup nor shutdown, however, most tutorials don't note that Bukkit already prints those messages in the chat for some time now : D

    For example:
    Code:
    [Lockette] Loading Lockette v1.7.4
    [iConomy] Loading iConomy v7.0 
    [ChestShop] Loading ChestShop v3.50t0012
    
    those are messages about plugin loading, but not calling onEnable() yet.

    Now, these:
    Code:
    [Lockette] Enabling Lockette v1.7.4
    [iConomy] Enabling iConomy v7.0
    [ChestShop] Enabling ChestShop v3.50t0012
    
    Are printed just before onEnable()

    Same on shutdown:
    Code:
    [ChestShop] Disabling ChestShop v3.50t0012
    [iConomy] Disabling iConomy v7.0
    [Lockette] Disabling Lockette v1.7.4
    
     
  21. Offline

    Digi

    A message, not so much... but if you have 50 plugins and each one of them sends at least an extra message, on PCs, that translates to a few seconds which actually is significant, expecially if you also play from the same machine.

    And why do extra and pointless work when it's not needed, people need to know :p The main problem is the outdated tutorials, but that's another story.
     
    justcool393 likes this.
  22. Offline

    jacklin213

    i c them all the time =.= so missleading especially when you watch the popular videos which Still have the EventHandler in the main class(without implementing the listener)<<< and that isnt included too

    OnTopic: The name , version , main in plugin yml is wrong (main is CASE SENSITIVE) i c a lot of plugins not working because of this
     
  23. Offline

    Sagacious_Zed Bukkit Docs

    It could slow it down. Logging can be quite expensive, especially when the call needs to block.'


    In the case of storing the player, most of the time I am checking to see if the player is in the collection, and not fetching the player from the collection. The other case is retrieving the value for the player from the collection. If you know enough that getPlayer and getPlayerExact are expensive, you probably know enough to clean up your collections, but that may be too much to assume.
     
    justcool393 likes this.
  24. Offline

    chaseoes Retired Staff

    Using PlayerCommandPreprocessEvent for completely regular and normal commands?
     
    Plo124 and makskay like this.
  25. Offline

    desht

    Using location.distance() inside a loop

    Commonly seen in code that draws circles, spheres etc. - using getDistance() to check if a block is inside a radius makes a call to Math.sqrt(), which is not cheap, and really inefficient if done inside a loop. Instead, calculate the square of your desired distance outside your loop, and using location.distanceSquared().
     
  26. Offline

    ferrybig

    compiling and running the server whit 2 differend versions of bukkit
    Doing this can result in unexcepted errors, like MethodeNotFound errors, or other errors
     
  27. Offline

    jacklin213

    Appently according to wulfspider if you compile with java 7 it won't be compatible with java 6
     
  28. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    You haven't even read this thread have you.
     
    EmeraldRailsMC and TwistedMexi like this.
  29. Offline

    jacklin213

    i have bad memory ^o^
     
  30. Offline

    deltahat

    I've got one: Not testing your plugin's generated help pages. Command help pages are created automatically from command registrations in the plugin.yml. If these registrations are incomplete the help pages won't be meaningful and won't automatically hide if the player lacks the permissions needed to run a command.
     
Thread Status:
Not open for further replies.

Share This Page