Fix SonarCloud: Code style — lambdas, @Override, collapsible ifs#2812
Fix SonarCloud: Code style — lambdas, @Override, collapsible ifs#2812tastybento merged 29 commits intodevelopfrom
Conversation
… fixes Fix ~128 SonarCloud Medium/Low severity issues: - S1612: Replace 53 lambda expressions with method references (28 files) - S1161: Add 23 missing @OverRide annotations (15 files) - S1066: Merge 8 collapsible if-statements - S1854/S1128/S1481/S1905: Remove useless assignments, imports, variables, casts - S6126: Convert 7 string concatenations to text blocks - S6204: .collect(Collectors.toList()) to .toList() - S1488: Return expressions directly - S3824: Use computeIfAbsent pattern - S106: Replace System.err with logger - Other minor style fixes (S1124, S1319, S1602, S1611, S1659, S1940) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR targets SonarCloud style findings across the BentoBox codebase, primarily through mechanical refactors (method references, missing @Override, collapsible ifs, and minor cleanups) and adds focused unit tests for ExpiringSet/ExpiringMap equality/hash behavior.
Changes:
- Refactors code to satisfy SonarCloud rules (method references, simplified control flow, interface types, direct returns, etc.).
- Adds new tests for
ExpiringSetandExpiringMapequals/hashCode. - Performs small API/behavior-adjacent refactors in a few core classes (notably command validation and a public getter signature).
Reviewed changes
Copilot reviewed 75 out of 75 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/world/bentobox/bentobox/util/ExpiringSetTest.java | Adds equals/hashCode tests for ExpiringSet (with proper closing). |
| src/test/java/world/bentobox/bentobox/util/ExpiringMapTest.java | Adds equals/hashCode tests for ExpiringMap (needs more robust shutdown handling on failures). |
| src/test/java/world/bentobox/bentobox/panels/settings/SettingsTabTest.java | Removes unused local variable in testGetIcon(). |
| src/test/java/world/bentobox/bentobox/managers/island/NewIslandTest.java | Adds @Override annotations; converts lambdas to method references; minor whitespace cleanup. |
| src/test/java/world/bentobox/bentobox/managers/island/IslandGridTest.java | Adds @Override annotations to lifecycle methods. |
| src/test/java/world/bentobox/bentobox/managers/island/IslandCacheTest.java | Converts static-mock lambdas to method refs; tightens Mockito matcher with eq(...). |
| src/test/java/world/bentobox/bentobox/managers/island/DefaultNewIslandLocationStrategyTest.java | Adds @Override; whitespace cleanup. |
| src/test/java/world/bentobox/bentobox/managers/RanksManagerTest.java | Converts static-mock lambdas to method refs; tightens matcher with eq(...). |
| src/test/java/world/bentobox/bentobox/managers/PlayersManagerTest.java | Fixes annotation placement; converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/managers/IslandsManagerTest.java | Method refs for Bukkit statics; switches expected message to a text block. |
| src/test/java/world/bentobox/bentobox/managers/IslandWorldManagerTest.java | Adds @Override; converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/managers/FlagsManagerTest.java | Adds @Override; converts Bukkit static mock to method reference. |
| src/test/java/world/bentobox/bentobox/managers/BlueprintClipboardManagerTest.java | Converts JSON strings to text blocks; adds @Override annotations. |
| src/test/java/world/bentobox/bentobox/managers/AddonsManagerTest.java | Converts YAML strings to text blocks in tests. |
| src/test/java/world/bentobox/bentobox/listeners/teleports/PlayerTeleportListenerTest.java | Removes unused variable. |
| src/test/java/world/bentobox/bentobox/listeners/flags/worldsettings/CleanSuperFlatListenerTest.java | Converts static-mock lambda to method reference. |
| src/test/java/world/bentobox/bentobox/listeners/flags/settings/PVPListenerTest.java | Converts static verify lambdas to method references. |
| src/test/java/world/bentobox/bentobox/listeners/flags/settings/MobTeleportListenerTest.java | Adds @Override annotation. |
| src/test/java/world/bentobox/bentobox/listeners/flags/protection/RaidTriggerListenerTest.java | Removes unused JUnit assertion imports. |
| src/test/java/world/bentobox/bentobox/listeners/flags/protection/InventoryListenerTest.java | Reorders modifiers to static final. |
| src/test/java/world/bentobox/bentobox/listeners/flags/protection/FireListenerTest.java | Removes unused MockedStatic local; keeps static mocking call. |
| src/test/java/world/bentobox/bentobox/listeners/JoinLeaveListenerTest.java | Converts online players static mock to method reference. |
| src/test/java/world/bentobox/bentobox/blueprints/BlueprintPasterTest.java | Converts static verify lambdas to method references. |
| src/test/java/world/bentobox/bentobox/api/flags/clicklisteners/CycleClickTest.java | Converts static mock to method ref; tightens Mockito matchers with eq(...). |
| src/test/java/world/bentobox/bentobox/api/flags/FlagTest.java | Converts static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/island/team/IslandTeamInviteCommandTest.java | Converts Bukkit.getItemFactory() static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/island/team/IslandTeamInviteAcceptCommandTest.java | Converts static mock lambda to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/island/IslandSpawnCommandTest.java | Converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/island/IslandResetCommandTest.java | Removes unused matcher import; converts scheduler/static builder mocks to method references. |
| src/test/java/world/bentobox/bentobox/api/commands/island/IslandGoCommandTest.java | Converts scheduler/plugin-manager static mocks to method references. |
| src/test/java/world/bentobox/bentobox/api/commands/island/IslandCreateCommandTest.java | Converts static builder mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/island/IslandBanCommandTest.java | Converts getOnlinePlayers() static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeCommandTest.java | Converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminSettingsCommandTest.java | Converts static mocks to method references. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminResetHomeCommandTest.java | Converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminResetFlagsCommandTest.java | Converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminRegisterCommandTest.java | Adds @Override annotation for teardown override. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminMaxHomesCommandTest.java | Converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminGetrankCommandTest.java | Converts statics to method references; adds @Override; whitespace cleanup. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/AdminDeleteCommandTest.java | Adds @Override; converts scheduler static mock to method reference; whitespace cleanup. |
| src/test/java/world/bentobox/bentobox/api/commands/DelayedTeleportCommandTest.java | Converts Bukkit statics to method references. |
| src/test/java/world/bentobox/bentobox/api/commands/DefaultHelpCommandTest.java | Converts scheduler static mock to method reference. |
| src/test/java/world/bentobox/bentobox/api/addons/AddonTest.java | Adds @Override on setup override. |
| src/test/java/world/bentobox/bentobox/api/addons/AddonClassLoaderTest.java | Adds @Override on setup override. |
| src/test/java/world/bentobox/bentobox/TestBentoBox.java | Alters IslandsManager static mocking setup (currently problematic due to unhandled MockedStatic). |
| src/test/java/world/bentobox/bentobox/RanksManagerTestSetup.java | Adds @Override; converts static mocks to method references; tightens Mockito matchers. |
| src/test/java/world/bentobox/bentobox/CommonTestSetup.java | Converts scheduler static mock to method reference; minor lambda cleanup. |
| src/main/java/world/bentobox/bentobox/util/Util.java | Simplifies boolean logic in isVersionCompatible. |
| src/main/java/world/bentobox/bentobox/util/ItemParser.java | Removes unused import/variables; clarifies TODO. |
| src/main/java/world/bentobox/bentobox/util/ExpiringSet.java | Collapses scheduled task lambda into one line. |
| src/main/java/world/bentobox/bentobox/util/ExpiringMap.java | Adds missing @Override on computeIfAbsent. |
| src/main/java/world/bentobox/bentobox/nms/CopyWorldRegenerator.java | Collapses nested if statements for readability. |
| src/main/java/world/bentobox/bentobox/managers/island/IslandGrid.java | Changes getGrid() return type to Map (source-breaking for callers expecting TreeMap). |
| src/main/java/world/bentobox/bentobox/managers/island/IslandCache.java | Replaces Collectors.toList() with toList(). |
| src/main/java/world/bentobox/bentobox/managers/island/DefaultNewIslandLocationStrategy.java | Removes unnecessary casts to double. |
| src/main/java/world/bentobox/bentobox/managers/IslandsManager.java | Simplifies direct return. |
| src/main/java/world/bentobox/bentobox/managers/IslandDeletionManager.java | Removes unused import. |
| src/main/java/world/bentobox/bentobox/managers/AddonsManager.java | Collapses nested if statements. |
| src/main/java/world/bentobox/bentobox/listeners/flags/protection/HurtingListener.java | Collapses nested if statements. |
| src/main/java/world/bentobox/bentobox/listeners/flags/protection/EggListener.java | Collapses nested if statements. |
| src/main/java/world/bentobox/bentobox/listeners/flags/protection/BreakBlocksListener.java | Minor condition simplification; removes redundant cast in reflection invoke. |
| src/main/java/world/bentobox/bentobox/listeners/PanelListenerManager.java | Uses interface type (Map) for static field. |
| src/main/java/world/bentobox/bentobox/hooks/ZNPCsPlusHook.java | Simplifies direct return. |
| src/main/java/world/bentobox/bentobox/database/json/adapters/TagTypeAdapterFactory.java | Removes unchecked cast (currently likely breaks compilation). |
| src/main/java/world/bentobox/bentobox/database/json/adapters/LocationTypeAdapter.java | Removes redundant casts when writing yaw/pitch. |
| src/main/java/world/bentobox/bentobox/blueprints/dataobjects/BlueprintTrialSpawner.java | Replaces System.err with plugin logging; switches to toList(). |
| src/main/java/world/bentobox/bentobox/blueprints/dataobjects/BlueprintEntity.java | Removes unnecessary cast. |
| src/main/java/world/bentobox/bentobox/api/panels/TemplatedPanel.java | Collapses nested if statements. |
| src/main/java/world/bentobox/bentobox/api/flags/Flag.java | Uses computeIfAbsent to initialize world flags and persist settings. |
| src/main/java/world/bentobox/bentobox/api/commands/island/IslandSetnameCommand.java | Removes color-translation in canExecute (introduces validation bug with & codes). |
| src/main/java/world/bentobox/bentobox/api/commands/island/IslandGoCommand.java | Lambda simplification (thenAccept). |
| src/main/java/world/bentobox/bentobox/api/commands/island/IslandCreateCommand.java | Collapses nested if statements. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/team/AdminTeamSetownerCommand.java | Adds missing @Override on execute. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeRegionsCommand.java | Simplifies delete logic; updates types to match IslandGrid#getGrid() return type; minor variable cleanup. |
| src/main/java/world/bentobox/bentobox/api/addons/AddonClassLoader.java | Refactors local class cache lookup using computeIfAbsent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // IslandsManager static | ||
| mockedStaticIM = Mockito.mockStatic(IslandsManager.class, Mockito.RETURNS_MOCKS); | ||
| //PowerMockito.mockStatic(IslandsManager.class, Mockito.RETURNS_MOCKS); | ||
| Mockito.mockStatic(IslandsManager.class, Mockito.RETURNS_MOCKS); | ||
| when(plugin.getCommandsManager()).thenReturn(cm); |
There was a problem hiding this comment.
Mockito.mockStatic(IslandsManager.class, ...) is called without keeping the returned MockedStatic handle. This makes it impossible to close explicitly and also leaves MockedStatic as an unused import in this file (which is a compilation error). Store the result in a field/local (e.g. mockedStaticIM) and close it in tearDown() (or remove the import if you switch to try-with-resources).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ExpiringMap<String, Integer> other = new ExpiringMap<>(1, TimeUnit.HOURS); | ||
| other.put("a", 1); | ||
| other.put("b", 2); | ||
|
|
||
| assertEquals(map, other); | ||
| assertEquals(other, map); | ||
| other.shutdown(); | ||
| } |
There was a problem hiding this comment.
This test creates a new ExpiringMap and calls other.shutdown() only at the end. If an assertion fails before the shutdown call, scheduler thread(s) may be left running and leak into subsequent tests. Use try/finally around other so shutdown() always executes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ExpiringMap<String, Integer> other = new ExpiringMap<>(1, TimeUnit.HOURS); | ||
| other.put("b", 2); | ||
|
|
||
| assertNotEquals(map, other); | ||
| other.shutdown(); | ||
| } |
There was a problem hiding this comment.
This test creates a new ExpiringMap and calls other.shutdown() only at the end. If an assertion fails before the shutdown call, scheduler thread(s) may be left running and leak into subsequent tests. Use try/finally around other so shutdown() always executes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ExpiringMap<String, Integer> other = new ExpiringMap<>(1, TimeUnit.HOURS); | ||
| other.put("a", 99); | ||
|
|
||
| assertNotEquals(map, other); | ||
| other.shutdown(); | ||
| } |
There was a problem hiding this comment.
This test creates a new ExpiringMap and calls other.shutdown() only at the end. If an assertion fails before the shutdown call, scheduler thread(s) may be left running and leak into subsequent tests. Use try/finally around other so shutdown() always executes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ExpiringMap<String, Integer> other = new ExpiringMap<>(1, TimeUnit.HOURS); | ||
| other.put("x", 10); | ||
| other.put("y", 20); | ||
|
|
||
| assertEquals(map, other); | ||
| assertEquals(map.hashCode(), other.hashCode()); | ||
| other.shutdown(); | ||
| } |
There was a problem hiding this comment.
This test creates a new ExpiringMap and calls other.shutdown() only at the end. If an assertion fails before the shutdown call, scheduler thread(s) may be left running and leak into subsequent tests. Use try/finally around other so shutdown() always executes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
src/main/java/world/bentobox/bentobox/managers/island/IslandGrid.java
Outdated
Show resolved
Hide resolved
src/main/java/world/bentobox/bentobox/database/json/adapters/TagTypeAdapterFactory.java
Outdated
Show resolved
Hide resolved
| // Naming the island - join all the arguments with spaces. | ||
| String name = String.join(" ", args); | ||
|
|
||
| // Check if the name isn't too short or too long | ||
| if (name.length() < getSettings().getNameMinLength() || Util.stripColor(name).isEmpty()) { | ||
| user.sendMessage("commands.island.setname.name-too-short", TextVariables.NUMBER, String.valueOf(getSettings().getNameMinLength())); | ||
| return false; | ||
| } | ||
| if (name.length() > getSettings().getNameMaxLength()) { | ||
| user.sendMessage("commands.island.setname.name-too-long", TextVariables.NUMBER, String.valueOf(getSettings().getNameMaxLength())); | ||
| return false; | ||
| } | ||
|
|
||
| // Apply colors | ||
| if (user.hasPermission(getPermissionPrefix() + "island.name.format")) { | ||
| name = ChatColor.translateAlternateColorCodes('&', name); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
canExecute validates emptiness using Util.stripColor(name), but color translation (& -> §) was removed from this method and now happens only in execute. Since stripColor only removes § codes, a player with island.name.format can pass validation with a name that becomes empty after translation (e.g. just color codes). Apply the same translation in canExecute when the permission is present, or update validation to treat & color codes as formatting before the emptiness check.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ExpiringMap<String, Integer> other = new ExpiringMap<>(1, TimeUnit.HOURS); | ||
| assertEquals(map, other); | ||
| assertEquals(other, map); | ||
| other.shutdown(); | ||
| } |
There was a problem hiding this comment.
This test creates a new ExpiringMap and calls other.shutdown() only at the end. If an assertion fails before the shutdown call, scheduler thread(s) may be left running and leak into subsequent tests. Use try/finally around other so shutdown() always executes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@tastybento I've opened a new pull request, #2818, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #2819, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #2820, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #2821, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #2822, to work on those changes. Once the pull request is ready, I'll request review from you. |
…id.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…agTypeAdapterFactory.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@tastybento I've opened a new pull request, #2823, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #2824, to work on those changes. Once the pull request is ready, I'll request review from you. |
…952-b6e1-453d-aa9d-2387f3edf481 [WIP] Update code style based on SonarCloud feedback
…is always called Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…e-work [WIP] Fix code style improvements based on SonarCloud feedback
Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…always runs Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…lation Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…ore-time Fix scheduler thread leak in ExpiringMapTest when assertions fail
…gain Fix scheduler thread leaks in ExpiringMapTest
Ensure ExpiringMap.shutdown() always runs in tests via try/finally
Fix leaked MockedStatic handle for IslandsManager in TestBentoBox
… testNotEqualsDifferentValues Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
…er-one Ensure ExpiringMap.shutdown() always runs in tests via try/finally
|
Summary
@Overrideannotations across 15 files (S1161)ifstatements (S1066).toList()(S6204), direct returns (S1488),computeIfAbsent(S3824), logger usage (S106), interface types (S1319)Risk
Low — mechanical refactors, no behavior change.
Test plan
./gradlew clean testpasses🤖 Generated with Claude Code