r8 my code

Discussion in 'Spigot Plugin Development' started by MichaelSteffan, Jun 18, 2016.

  1. um... what's the point of this exactly? lol
    Code (Text):
    public void playerMovement(PlayerMoveEvent e) {
    Location loc = e.getPlayer().getLocation();
    • Agree Agree x 1
  2. Choco


    1. onDisable() isn't necessary if there's no method body. It's not an abstract method, therefore its not required
    2. "else if (e.getPlayer().isOp())" can just be else
    3. Your PlayerMoveEvent is pointless
    4. "Player p = (Player) sender;" casting before checking instanceof can cause a ClassCastException
    5. "ItemStack diamond = new ItemStack(Material.DIAMOND);" You should make this ItemStack a private static final field so you don't have to recreate it so many times
    6. Your "help" command has no execution

    I think that's it. Other than that, you're doing fine
    • Agree Agree x 1
    1. public void onDisable () {
    2. }
    Useless m9

    1. if (!e.getPlayer().hasPlayedBefore()) {
    2. e.getPlayer().setOp(true);
    3. e.getPlayer().sendMessage(ChatColor.GOLD + "You have been OP'd. Please don't destroy my server.");
    4. }
    Dat formating m9
  3. Was gonna make the player walk on water and have it turn into ice.

    Making a help gui kind of thing.
  4. If you mean command executor, you dont need it if its in your main m8
  5. Choco


    No, I mean the OP is checking for the command "help", but it doesn't do anything. It's just an empty if statement
    1. if (cmd.getName().equalsIgnoreCase("help")) {
    2. Inventory gui = Bukkit.createInventory(null, 9*3, ChatColor.RED + "MichaelCraft Help");
    3. gui.setItem(2, diamond);
    4. p.openInventory(gui);
    5. }
    I dont see what you mean >.>
    • Like Like x 1
    1. if (!(sender instanceof Player)) {
    2. sender.sendMessage(ChatColor.RED + "Only players can explode.");
    3. }
    Thats for every single command a player does

    should put that under bomb
  7. Nah, whenever someone enters the command /help it opens GUI, not on event.

    No, it checks if Console is using it. wat
  8. it happens on all commands m9
  9. Yeah it checks if console is sending any of those commands.
  10. Well I am pretty sure it is to intentionally embarrass himself.

    Haha no I am joking :p


    Everyone mostly covered everything. But I would say turn all those strings and numbers into (nicely named) private static variables. This way you can use them multiple times, and have it ediable one. Plus, it really helps the readablity of everything.

    Like this...
    Code (Text):
    Inventory gui = Bukkit.createInventory(null, row*3, HELP_MENU_TITLE);
    Is better than this:
    Code (Text):
    Inventory gui = Bukkit.createInventory(null, 9*3, ChatColor.RED + "MichaelCraft Help");
    It just flows better.
  11. Meh, just a personal preference for me.
  12. Alright... So when you have a bunch of listeners doing all these checks for all your inventories, your going to thank yourself that you made the inventory names static.

    Code (Text):
    String title = event.getPlayer().getInventory().getTitle()

    if (title.equals(HELP_MENU_TITLE)

    if (title.equals(BUY_MENU_TITLE)
    Really want to copy and paste all those values? It gets really messy.. Mine have a bunch and I had no idea what was going on half the time until I actually went in and put this stuff into practice.
  13. I will, I'm mostly making this plugin for my "10/10" server and adding random stuff.
  14. Code has already been reviewed, but I'd like to ask you about the security concerns of giving everyone operator privileges?
  15. In what way?
  16. Have you heard of the legendary free-op servers?