1.12.2 2000 Lines ClickListener

Discussion in 'Spigot Plugin Development' started by TaskID, Aug 8, 2020.

  1. ok so, I've been working on a CityBuild Plugin for a while now. I have over 120 classes, I'd say it's relatively Object Oriented. And I use A LOT of GUI's. But one class got completely too much in it: The InventoryClickListener..
    This thing has 2000 lines of code, and even though I use a lot of comments, it's very confusing.
    So, how do I split that up? Should I create different Click Listeners for different inventorys? Is that going to be laggy?
    My question is, how do I handle a lot of Inventory Click Event checks with clearer classes but with good performance?

    And please, if you have no idea, don't answer. I need answers from pros that know exactly how to do it best.

    Edit: And I don't use repeating code in it, every system has their own classes and often a own package where I have methods to use in the Inventory Click Listener
     
  2. I use the InventoryClickListener only for checking which of my GUIs has been clicked. Every logic after that is handled by other classes. Do you use a custom GUI "overlay" class that holds multiple information including the Inventory object, or do you check just the Inventory objects?
     
  3. I firstly check the Inventory Titles, and then the Item Display Names, when that fits, I run my methods for whatever the Items should do after clicking
     
  4. That may work in the beginning (I did that too for a while), but it gets really messy over time. I searched up a lot on that topic and build my own structure based on ideas from others and some own thoughts and it is really useful.
    Checking inventories by titles is not ideal, better would be to check inventory objects. I wrap my Inventory objects in a (abstract) "SimpleGUI" class alongside some other variables. Furthermore instead of checking the item display name (rather than that you should use ItemStack#isSimilar), I have another class containing ItemStack, the slot it's in and, optionally, a onClick method. That allows me to create code that is later executed in the click listener. I also heavily build more "wrapping classes" on top of that to make creating and working with GUI's easier.
    If you have a lot of GUI's it is probably going to be useful for you as well, but to help further we'd need more insight about the different GUI's. For example I have multiple GUI's where user can modify certain numbers of an object, so I build a highly specific GUI structure on top of the "baseline" just for that. Do note that that takes time and effort, I spent a good 3 weeks just on planning and building it, though more experienced developers may take less.
     
    #4 wand555, Aug 8, 2020
    Last edited: Aug 8, 2020
    • Friendly Friendly x 1
  5. Ok thank you! :) But do you save your inventory objects in HashMaps or how can I imagine that?
     
  6. There are libraries out there that can handle inventory gui abstractions for you. I created one too (it's in my signature), but stefvanschie's IF is a more popular choice.
     
  7. That is one of the few things I still nag about a little bit... Overall I only have 5 different "types" of GUI's so I just check the manually each. This is performance wise probably not the best solution, but the plugin is designed that the user configures everything before playing, so it doesn't really matter here.
    Code (Java):
        @EventHandler
        public void onGUIClickEvent(InventoryClickEvent event) {
            Inventory clickedInv = event.getClickedInventory();
            if(clickedInv == null) return;
            if(event.getCurrentItem() == null || event.getCurrentItem().getType() == Material.AIR) return;
            if(!(event.getWhoClicked() instanceof Player)) return;
            Player player = (Player) event.getWhoClicked();
            //if click was in the main gui with all challenges
            if(challengeManager.getMainGUI().equalsInv(clickedInv)) {
                event.setCancelled(true);
                challengeManager.getMainGUI().onClick(event.getRawSlot(), player);
                return;
            }

            {
                Challenge challenge = numberConfigurableGUIClick(clickedInv);
                if(challenge != null) {
                    ChangeValueGUI gui = ((NumbersConfigurable) challenge).getChangeValueGUI();
                    event.setCancelled(true);
                    gui.onClick(event.getRawSlot(), player, challenge);
                }
            }

            {
                Challenge challenge = punishableGUIClick(clickedInv);
                if(challenge != null) {
                    PunishmentGUI gui = ((Punishable) challenge).getPunishmentGUI();
                    event.setCancelled(true);
                    gui.onClick(event.getRawSlot(), player, challenge);
                }
            }

            {
                Challenge challenge = punishableNumberConfigurableGUIClick(clickedInv);
                if(challenge != null) {
                    ChangeValueGUI gui = ((NumbersConfigurable) ((Punishable) challenge).getPunishment()).getChangeValueGUI();
                    event.setCancelled(true);
                    gui.onClick(event.getRawSlot(), player, challenge);
                }
            }

            {
                Challenge challenge = occurableLimitGUIClick(clickedInv);
                if(challenge == null) return;
                ChangeValueGUI gui = ((OccurrableLimit) challenge).getLimitGUI();
                if(gui == null) return;
                event.setCancelled(true);
                gui.onClick(event.getRawSlot(), player, challenge);
            }

        }

        private Challenge numberConfigurableGUIClick(Inventory inventory) {
            for(Challenge challenge : Challenge.getChallenges()) {
                if(challenge instanceof NumbersConfigurable) {
                    ChangeValueGUI gui = ((NumbersConfigurable) challenge).getChangeValueGUI();
                    if(gui.equalsInv(inventory)) return challenge;
                }
            }
            return null;
        }

        private Challenge punishableGUIClick(Inventory inventory) {
            for(Challenge challenge : Challenge.getChallenges()) {
                if(challenge instanceof Punishable) {
                    PunishmentGUI gui = ((Punishable) challenge).getPunishmentGUI();
                    if(gui.equalsInv(inventory)) return challenge;
                }
            }
            return null;
        }

        private Challenge punishableNumberConfigurableGUIClick(Inventory inventory) {
            for(Challenge challenge : Challenge.getChallenges()) {
                if(challenge instanceof Punishable) {
                    Punishment punishment = ((Punishable) challenge).getPunishment();
                    if(punishment instanceof NumbersConfigurable) {
                        if(((NumbersConfigurable) punishment).getChangeValueGUI().equalsInv(inventory)) return challenge;
                    }
                }
            }

            return null;
        }

        private Challenge occurableLimitGUIClick(Inventory inventory) {
            for(Challenge challenge : Challenge.getChallenges()) {
                if(challenge instanceof OccurrableLimit) {
                    ChangeValueGUI gui = ((OccurrableLimit) challenge).getLimitGUI();
                    if(gui.equalsInv(inventory)) return challenge;
                }
            }
            return null;
        }
     
  8. Umm, I just found a "Localized Name" for ItemMetas. What is that? Is this like a "hidden" name? If yes it would be a great base for GUI Items ^^ Just wondering what that is bc I found nothing on Google
     
  9. I don't really know, never used them. Overall I prefer to use the slot as the identificator, but others prefer the itemstack.
     
  10. Personally I wrap my invs in a class which handles clicks. Each of my inventories items have 2 unique variables attributed in nbt tags. The inventory id and the item command. So essentially to tell what inventory im handling, I pass it through the click listener and if it is applicable to any of my registered inventories I pass it to that handler. No need to check names and all data is hidden from player meaning I can identify the difference between two nameless visually identical items.
    If you make your inventory builder abstract it means whatever inherits that class will also be forced to inherit it’s functions.
    I found it was better to identify an inventory by the items inside rather than an identifier attached to the inventory it’s self. It’s much more seemless

    edit— just to clarify the command nbt for each item is just to pass through a switch. It’s much easier to get the nbt tag which tells me exactly what the item does instead of matching the item type,name, description etc etc
     
    • Friendly Friendly x 1