Refactor AdapterListener to Eliminate Duplicate Adapter Calls #164
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors event and button dispatch to ensure each adapter or button callback is invoked exactly once, replacing redundant lambda-based loops with single enhanced for-loops.
- Replaced
forEachlambdas inAdapterListenerwith explicit for-each loops to prevent double invocation. - Applied the same loop style in
InventoryDefaultto handle button callbacks consistently. - Removed any duplicated iterations over adapters and buttons.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/java/fr/maxlego08/menu/listener/AdapterListener.java | Replaced plugin.getListenerAdapters().forEach calls with single for-each loops |
| src/main/java/fr/maxlego08/menu/inventory/inventories/InventoryDefault.java | Replaced buttons.forEach calls with enhanced for-loops for inventory opening/building |
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onInventoryClick(event, (Player) event.getWhoClicked()); | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onDrag(InventoryDragEvent event) { | ||
| if (event.getWhoClicked() instanceof Player) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onInventoryDrag(event, (Player) event.getWhoClicked())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onInventoryDrag(event, (Player) event.getWhoClicked()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onClose(InventoryCloseEvent event) { | ||
| if (event.getPlayer() instanceof Player) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onInventoryClose(event, (Player) event.getPlayer())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onInventoryClose(event, (Player) event.getPlayer()); |
There was a problem hiding this comment.
You’re repeatedly casting event.getWhoClicked() to Player inside the loop; consider extracting it to a local Player variable before the loop to improve readability and avoid redundant casts.
| @EventHandler | ||
| public void onConnect(PlayerJoinEvent event) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onConnect(event, event.getPlayer())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onConnect(event, event.getPlayer()); | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onQuit(PlayerQuitEvent event) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onQuit(event, event.getPlayer())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onQuit(event, event.getPlayer()); | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onInventoryClick(InventoryClickEvent event) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onInventoryClick(event, (Player) event.getWhoClicked())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onInventoryClick(event, (Player) event.getWhoClicked()); | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onDrag(InventoryDragEvent event) { | ||
| if (event.getWhoClicked() instanceof Player) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onInventoryDrag(event, (Player) event.getWhoClicked())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onInventoryDrag(event, (Player) event.getWhoClicked()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onClose(InventoryCloseEvent event) { | ||
| if (event.getPlayer() instanceof Player) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onInventoryClose(event, (Player) event.getPlayer())); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onInventoryClose(event, (Player) event.getPlayer()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @EventHandler | ||
| public void onPick(EntityPickupItemEvent event) { | ||
| if (event.getEntity() instanceof Player player) { | ||
| this.plugin.getListenerAdapters().forEach(adapter -> adapter.onPickUp(event, player)); | ||
| for (ListenerAdapter adapter : this.plugin.getListenerAdapters()) { | ||
| adapter.onPickUp(event, player); | ||
| } |
There was a problem hiding this comment.
All event handlers follow the same iteration pattern; consider extracting this loop into a helper method (e.g., dispatchEvent) to reduce duplicated code and improve maintainability.
| for (Button button : this.buttons) { | ||
| button.onInventoryOpen(player, this, placeholders); | ||
| } |
There was a problem hiding this comment.
[nitpick] This manual loop over buttons appears multiple times; consider abstracting button processing into a reusable helper or using concise method references for cleaner code.
| for (Button button : this.buttons) { | |
| button.onInventoryOpen(player, this, placeholders); | |
| } | |
| processButtons(button -> button.onInventoryOpen(player, this, placeholders)); |
|
I don’t really understand your pull request, what is the goal? Because here the changes do not seem relevant to me |
|
The goal is to replace lambdas with regular for loops because lambdas can be less performant when dealing with large data sets and high-frequency events. On a server with 500+ players, this small difference adds up — as seen in the attached usage snapshot from zMenu. 
|
|
Have you more concrete details about the effect of this modification? any stats or else |
|
I haven't tested for zmenu yet, but I solve every plugin that is open source and uses lambda like this and has high usage, and their usage solves incredibly, I can give an example of the duel plugin right now, the pr I opened for duel is below after: |
|
after: |
2 participants
This PR removes redundant calls to
adapter.onX()methods inAdapterListener. Previously, each event handler was iterating overplugin.getListenerAdapters()twice—once using aforEachlambda and again using a traditional for-each loop—which caused each adapter to process the same event twice.Changes:
forloops following eachforEachinvocation.This cleanup improves performance and prevents potential side effects due to double event handling.