Solved Would it be bad?

Discussion in 'Spigot Plugin Development' started by MisterFantasy, May 2, 2017.

  1. Hello, I have a question,

    Would it be bad to create a custom inventory like this:
    Code (Text):

     
        void createInventory(Player p, String title, int size, String material, String name, int amount, short data)
        {
            Inventory inv = Bukkit.createInventory(null, size, title);
           
            for (int i = 0; i < size; i++)
            {
                if (this.getMain().getConfig().contains(i + ""))
                {
                    ItemStack x = new ItemStack(Material.getMaterial(material), amount, (short) data);
                    ItemMeta xMeta = x.getItemMeta();
                    xMeta.setDisplayName(name);
                    x.setItemMeta(xMeta);
                   
                    inv.setItem(i, x);  
                }
            }
            p.openInventory(inv);
        }
       
        }
     
    #1 MisterFantasy, May 2, 2017
    Last edited: May 2, 2017
  2. I don't think there's anything bad about it. Note that that code will be extremely sensitive to null pointer exceptions though.
     
  3. no, i think this is pretty good :)
     
  4. Choco

    Moderator

    Yes, because you're missing a lot of key concepts:
    1. You're calling this.getMain().getConfig() way too much for it to be remotely okay. Create a local variable
    2. Your #getX() calls to retrieve data from the configuration file are missing periods. i.e. you're doing i + "Amount" rather than i + ".Amount"
    3. The inventory is being opened every single time a new item is added
    4. You're passing variables to the method just to set them? Why...? Why not just create variables within the method ._. Don't pass parameters you don't need
    5. The values retrieved from the configuration file may be null if they're not found. Provide some default values to prevent any NPE's
     
    • Winner Winner x 2
  5. Oh wow, I totally missed #3, that's a really bad thing yes
     
  6. I have adjusted the code for it now, would this be okay?
     
  7. Choco

    Moderator

    1. Why not pass a Material parameter for "material"?
    2. Any reason to have the "this.getMain().getConfig().contains(i + ""))" check? Seems rather redundant to me
    3. Why are you casting "data" to a short when you're already passing a short to the method?

    Everything else should be fine, though
     
  8. #1 The material is a String because I want to be able to set the name as a string and not get the material via Material.<>
    #2 That check is to make sure that 'i' so it won't fill in spots that are not set in the config.
    #3 True, I shouldn't be doing that.
     
  9. Code (Text):
        void createInventory(Player p, String title, int size, String material, String name, int amount, short data)
        {
            Inventory inv = Bukkit.createInventory(null, size, title);
           
            for (int i = 0; i < size; i++)
            {
                if (this.getMain().getConfig().contains(i + ""))
                {
                    ItemStack x = new ItemStack(Material.getMaterial(material), amount, data);
                    ItemMeta xMeta = x.getItemMeta();
                    xMeta.setDisplayName(name);
                    x.setItemMeta(xMeta);
                   
                    inv.setItem(i, x);  
                }
            }
            p.openInventory(inv);
        }
     
  10. Choco

    Moderator

    Yes, but you also have to realize that Material#getMaterial() can return null, and it would throw a NullPointerException when passed to the ItemStack constructor. Keep in mind, if you happen to have someone else use your method (i.e. in an API), I'm sure they would very much prefer to call a method with a Material constant rather than an arbitrary String with no definition. When you pass a String to that method, it could be anything. I could attempt to call the method by passing "2008ChocoIzAwesome", but it would just throw an NPE. Material is a lot more limiting and you can check if a Material is null before passing it to the method.

    Granted, if this method were for API use, I'm sure you'd have Javadoc comments, restrictions and null checks on the parameters passed into the method, but let's just ignore that for now. When I'm programming, my thought process is... "Would this make sense in an API from the perspective of another developer?"
     
  11. It's just for personal use right now.
     
  12. Choco

    Moderator

    Whatever you prefer. Just a strong recommendation to keep in consideration for the future. You may look back in the near future and ask yourself, "Why did I pass a String parameter to this method?". ¯\_(ツ)_/¯
     
  13. Haha! Thanks alot for your help!
     
    • Friendly Friendly x 1