Solved Inserting a key into a HashMap returns null - no null values in HashMap

Discussion in 'Spigot Plugin Development' started by Sploon, Sep 25, 2016.

  1. Hi Spigot - I'm a little confused.
    This is the code in question:
    Code (Text):
    Bukkit.broadcastMessage(ChatColor.stripColor(staticItemMap+"")); // Todo - remove tracing
    for (ItemStack i : staticItemMap.keySet()) {
       Bukkit.broadcastMessage(ChatColor.stripColor(i+" = "+staticItemMap.get(i))); // Todo - remove tracing
       if (i.equals(item)) {
           return staticItemMap.get(i);
       }
    }
    return null;
    Let me explain.
    So I have a HashMap that pairs an ItemStack to my own object (GUIItem). Originally the GUIItem extended ItemStack, but because of this line:
    Code (Text):
    this.getInventory().setItem(index, item != null && item.getTypeId() != 0?CraftItemStack.asNMSCopy(item):null);
    in the InventoryClickEvent, event.getCurrentItem() is never going to be an instance of GUIItem, as the ItemStack - GUIItem - is copied to an NMS "CraftItemStack" before being placed into the inventory. So when I fetch the clicked item, it returns that NMS version, not my custom version.
    (Correct me if I'm wrong, this has been a bit of a process).

    As a result, I just gave the GUIItem an ItemStack field. That's fine. I store the GUIItem in a HashMap (as seen in the code) - HashMap<ItemStack, GUIItem>.
    The next issue I had is when I tried to get the GUIItem associated with the ItemStack. Standard - staticItemMap.get(item);
    It returns null. I figured that maybe this was the whole == vs #equals() thing; == returns true if the two values are the same - aka the same object, stored in memory in the same place, whereas #equals() returns true if the two objects are equal. A HashMap stores a specific key (object) with a specific value, so even if the two objects were equal (but not the same), the HashMap would return null, right? That's my logic, anyways.
    So I added a for-loop that checks to see if each key in the HashMap is equal to the item clicked.
    Code (Text):
    for(ItemStack i: staticItemMap.keySet()) {
       if(i.equals(item)) { // are the two objects equal
          return staticItemMap.get(i); // fetch the GUIItem using its key
       }
       return null; // the item is not in the map
    }
    This seems fine, but it returns null.
    After a bit of tracing, I can confidently say that i does, in fact, equal item, and the
    Code (Text):
    return staticItemMap.get(i);
    is what returns null.
    That's where I am now. I've double (tripple) checked that the items are, without a doubt, being added to the HashMap (it wouldn't have gotten this far otherwise, but someone's going to say it).
    I've also printed out the contents of the HashMap:
    Code (Text):
    [INFO]: map: {ItemStack{WATCH x 1, UNSPECIFIC_META:{meta-type=UNSPECIFIC, display-name=LOBBY MANAGER}}[email protected], ItemSt
    ack{GRASS x 1, UNSPECIFIC_META:{meta-type=UNSPECIFIC, display-name=MAP MANAGER}}[email protected]}
    Finally, I've printed out each object in the HashMap individually, with both its key and its value:
    Code (Text):
    [INFO]: ItemStack{WATCH x 1, UNSPECIFIC_META:{meta-type=UNSPECIFIC, display-name=LOBBY MANAGER}} = null
    [INFO]: ItemStack{GRASS x 1, UNSPECIFIC_META:{meta-type=UNSPECIFIC, display-name=MAP MANAGER}} = null
    As you can see, getting the object with its key returns null, and I can't figure out why. This is what its doing:
    Code (Text):
    for(ItemStack item : staticItemMap.keySet()) {
       Bukkit.broadcastMessage(item+" = "+staticItemMap.get(item));
    }
    Thanks!
     
    #1 Sploon, Sep 25, 2016
    Last edited: Sep 25, 2016
  2. Can you provide a full snippet? You must be messing up some put call if you get keys with null values.
     
  3. This is the constructor:
    Code (Text):
    public GUIItem(ItemStack item, InventoryGUI gui, int page, int slot, boolean staticItem) {
         this.item = item;
         this.gui = gui;
         staticItemMap.put(item, this);
         if (staticItem)
             itemLocations.addAll(gui.addStaticItem(item, slot));
         else if (slot == -1)
             itemLocations.add(gui.addItem(item));
         else
             itemLocations.add(gui.setItem(item, page, slot));
    }
    This is where the code is run:
    Code (Text):
    if(gui == null) {
    //...some code
    } else {
       event.setCancelled(true);
       if (gui.getPermission().equals("") || player.hasPermission(gui.getPermission())) {
          GUIItem item = GUIItem.getGUIItem(event.getCurrentItem());
          Bukkit.broadcastMessage(item+""); // Todo - remove tracing
          if (item != null) {
             item.clickItem(player, event.getClickedInventory());
          }
       } else {
          player.sendMessage(ChatColor.RED + "You do not have permission to use this GUI.");
          player.closeInventory();
       }
    }
     
  4. If you using multiple classes, then you may want to check how the data is being passed through, I can't see it clearly enough from the above code.
     
  5. Values are added to the HashMap in the constructor, and fetched in the getGUIItem() method. That's it :/

    EDIT:
    Code (Text):
    public GUIItem(Material type, int amount, short damage, InventoryGUI gui, int page, int slot, boolean staticItem) {
       this(new ItemStack(type, amount, damage), gui, page, slot, staticItem);
    }
    Every GUIItem, so far, has gone through this constructor (which passes to the one from my last post).
     
    #5 Sploon, Sep 26, 2016
    Last edited: Sep 26, 2016
  6. try putting
    Code (Text):
    staticItemMap.put(item, this);
    after the if else statement in GUIITEM Constructor
     
  7. this can't be null, so you're doing something really funky with the Map (f.e. using two different maps which apparently contain the same keys).

    For starters, if the staticItemMap is static, it shouldn't be. Move that out of the item class and create a registry class which maintains the item -> GUI item mapping.
     
  8. The getGUI() method is called in the if/else statement, so the staticItemMap has to be placed earlier. It shouldn't make a difference, though, as the item variable is defined in the first line of the constructor.
    staticItemMap is static, yes. I can send you the class if you're interested.
    According to Intellij, these are all of the times the map is accessed:
    [​IMG]

    I don't see how there could be two maps there.
    As for a registry class (I've never heard of it before) is that this: https://docs.oracle.com/javase/7/docs/api/java/rmi/registry/Registry.html
    What is the advantage of using this, rather than a HashMap?
     
  9. A HashMap needs the hashCode method, so if you have two instances of the same class which should be considered equal and you do not implement your own hashcode, hashmap will not work, as we are back to something very similar to == vs equals()
     
  10. I check if the two objects are equal before calling the HashMap get method (but using its actual key). To clarify: If you look at the screenshot above, I use the key received from the keySet() to fetch a value, not the object equal to the key.
     
  11. you haven't heard of a registry class because I wasn't talking about an existing class, lol. I was talking about a class which handled the mapping externally (static is a dirty solution).
     
  12. That set uses the same back-end as the hashmap this does not make a difference.
     
  13. I figured static would be better than a singleton class to handle one variable. That's the point of static, isn't it? I'd probably just end up making the registry class a nested class, which would be static anyways. What's wrong with using static, though?
    I don't understand what you mean?
     
  14. Instead of looping over all keys, iterate over the entries using entrySet() which returns a Set<Map.Entry<ItemStack, GUIItem>>. It reduces lookup time and should prevent it from returning null. @Jo_Jo_2000 was right when they said you need a hashCode for a class if you want it to properly work in a HashMap. It's a shame the Bukkit API didn't implement it, probably cause NBT data regularly changes.
     
    • Useful Useful x 1
  15. @Sploon in order to improve performance, HashMaps use "buckets" which are groups of keys which have part of identical hashes. If a class doesn't have hashCode() implemented, it uses the location in memory which can be quite different between instances, so then the HashMap won't find the item in the correct bucket.
     
  16. Ahh okay, that makes sense.
    Thanks everyone for the help, using the Set<Entry> worked. Thanks again!
     
  17. Static in itself isn't bad, but using static to share variables across classes is not all that great (which holds for singletons as well). The point of static is solely to share data across all instances of that class. If you need to access data in multiple classes, a better approach is to store it in a single instance (f.e. the main class or listener class) as a non-static variable, and provide a getter method to get the data in other classes (whilst sharing the instance with other classes by f.e. using the constructor - otherwise known as [constructor] dependency injection)
     
  18. Actually that's what I usually do, but the staticItemMap is created and accessed in the GUIItem class, and nowhere else. I rarely use static with the exception of constants and Maps/Lists that hold objects of a certain type for that class (like the staticItemMap).