1.15.2 Go through source code.

Discussion in 'Spigot Plugin Development' started by Stjernholm, Jan 25, 2020.

  1. Don't know if this is the right subforum, or if there are any rules against this...

    But could anyone go through the source code below, and tell if this can be called "good code", or if there are beginner mistakes etc. within? (I'm unsure at this point.)
    https://github.com/StjernholmPW/DelayedSpawners
    This is my first public spigotmc.org resource, just want a push forward, and some improvement points I could work on.

    Thank you.
    Stjernholm
     
  2. Strahan

    Benefactor

    Well, being a Java noob I'm not really qualified to judge, but here are my thoughts:

    In the primary class, those enable/disable messages are pointless log clutter. Spigot already sends them.
    In the command class, the switch should be on args[0].toLowerCase() to prevent case issues and using returns would cut down on indentation. Not that it's a bad thing, just a matter of personal preference.
    Event classes do not use proper naming convention
    In EditSpawnerMenu, it would've been easier to use a collection instead of a whole bunch of switch cases
    I see from the config mgr you apparently aren't aware that the config get options allow for defaults, so instead of like
    Code (Text):
    public int getMinSpawnDelay(String entity) {
      if(valueExist(entity, "mindelay")) {
        return delayedSpawners.getConfig().getInt("type." + entity + ".mindelay");
      }
      return 200;
    }
    You could just do
    Code (Text):
    public int getMinSpawnDelay(String entity) {
      return delayedSpawners.getConfig().getInt("type." + entity + ".mindelay", 200);
    }
    Looks reasonable to me overall.
     
  3. not that important but packages should be all lowercase and in your pom you dont need the maven shade plugin since you dont shade any dependencies
     
  4. drives_a_ford

    Moderator

    The way you're implementing InventoryHolder is discourage:
    You can just keep track of your custom Inventory instances in a HashSet and check if the inventory in question is within said set.
     
    • Informative Informative x 1
  5. Just to clarify on this, does this mean that one should not create their own class implementing the InventoryHolder interface, and then setting their custom inventory's holder as their custom class, with the goal to later be able to check if an inventory is a custom inventory by using instanceof?

    This is only semi-relevant to OP, sorry for that, I'm asking for personal curiosity.

    Also, for OP:
    You could try using the final statement, it's pretty simple once you get a hang of it, it makes your code more readable and gives a small performance boost. :)
     
  6. drives_a_ford

    Moderator

    Yes, one should not create such classes as the quote (within the quote) clearly states.
     
    • Like Like x 1
  7. Thank you sir.
     
  8. Just nitpicking, but your packages should follow naming conventions and be lowercase
     
  9. Also comments are good helping hands for your future improvements. You never know when will you open your project again to add some functions or fix bugs. In this small project it isn't as mandatory as in huge projects but it still improves readability of the code. :)
     
    • Agree Agree x 1