Conversation
Prevent NullPointerException by checking defenseConfig and antiPushLogic before use. Extract repeated antiPushLogic.getIgnoredPlayers() call into a helper method to improve maintainability and avoid null access.
Check that the ally target is not null and is valid before returning it to prevent potential null pointer exceptions or invalid target references.
This improves readability and maintainability by using instanceof for type checking instead of direct class comparisons. The logic remains unchanged.
Only move to help group members when all three conditions are met: goToGroup enabled, respondAttacks enabled, and group is in help list. Additionally, optimize attack detection logic to avoid unnecessary movement checks when already targeting an enemy.
Improve readability and maintainability by moving the complex player filtering logic from friendUnderAttack into a dedicated shouldHelpPlayer method. This also removes unnecessary stream operations and list creation.
…Target The method name had a spelling error which could cause confusion and affect code readability.
Contributor
Reviewer's GuideRefactors DefenseMode’s attack detection and ally-help logic for clarity and robustness, adding null-safety around configuration/anti-push logic, simplifying module checks, and extracting reusable helper methods for filtering allies and ignored players. Updated class diagram for DefenseMode defense and ally-help logicclassDiagram
class DefenseMode {
- DefenseConfig defenseConfig
- AntiPushLogic antiPushLogic
- HeroAPI heroapi
- BotApi botApi
- Collection~Player~ players
- Ship target
+ onTickBehavior() void
- setup() void
- isUnderAttack() boolean
- hasPreviousTarget() boolean
- friendUnderAttack() boolean
- hasAttacker() boolean
- getTarget(ships Collection~Player~) Ship
- shouldHelpPlayer(player Player) boolean
- isIgnoredPlayer(playerId int) boolean
- goToMemberAttacked() void
- inGroupAttacked(id int) boolean
}
class DefenseConfig {
+ rangeForAttackedEnemy double
+ helpList Collection~HelpList~
+ helpAttack boolean
+ defendEvenAreNotEnemies boolean
+ goToGroup boolean
+ respondAttacks boolean
}
class AntiPushLogic {
+ getIgnoredPlayers() Collection~int~
+ registerTarget(target Ship) void
}
class HeroAPI {
+ getId() int
+ getMap() Map
+ isAttacking() boolean
+ distanceTo(entity Entity) double
+ getEntityInfo() EntityInfo
}
class BotApi {
+ getModule() Module
+ setModule(module Module) void
}
class Module
class DefenseModule
class PVPModule
class SentinelModule
class TemporalModule
class MapModule
class Player {
+ isValid() boolean
+ isAttacking() boolean
+ getTarget() Entity
+ getTargetAsShip() Ship
+ getEntityInfo() EntityInfo
+ getId() int
}
class Ship {
+ isValid() boolean
+ getId() int
+ getLocationInfo() LocationInfo
}
class Pet
class Npc
class SharedFunctions {
+ getAttacker(player Player, players Collection~Player~, hero HeroAPI, onlyEnemies boolean) Ship
}
class Entity
class EntityInfo {
+ getClanId() int
+ getClanDiplomacy() Diplomacy
+ isEnemy() boolean
}
class LocationInfo {
+ distanceTo(hero HeroAPI) double
}
class Diplomacy {
<<enumeration>>
ALLIED
}
class HelpList {
<<enumeration>>
CLAN
ALLY
GROUP
EVERYONE
}
DefenseMode --> DefenseConfig
DefenseMode --> AntiPushLogic
DefenseMode --> HeroAPI
DefenseMode --> BotApi
DefenseMode --> "*" Player
DefenseMode --> Ship
DefenseMode ..> SharedFunctions : uses
DefenseMode ..> HelpList : uses
DefenseMode ..> Diplomacy : uses
BotApi --> Module
DefenseModule --|> Module
PVPModule --|> Module
SentinelModule --|> Module
TemporalModule --|> Module
MapModule --|> Module
Player --> EntityInfo
Player --> Ship
Ship --> LocationInfo
EntityInfo --> Diplomacy
Pet --|> Player
Npc --|> Entity
Ship --|> Entity
Player --|> Entity
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The change from
botApi.getModule().getClass() != MapModule.classto!(currentModule instanceof MapModule)alters behavior for any subclasses ofMapModule(they will now be treated as MapModule), so please confirm this is intentional or narrow the condition if only the concrete class should be exempted. - In
isUnderAttack(), callinggoToMemberAttacked()only whenfriendUnderAttack()is true changes the previous behavior where group movement could be triggered even when dealing with your own attacker; it’s worth double-checking that this priority change matches the desired defense behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change from `botApi.getModule().getClass() != MapModule.class` to `!(currentModule instanceof MapModule)` alters behavior for any subclasses of `MapModule` (they will now be treated as MapModule), so please confirm this is intentional or narrow the condition if only the concrete class should be exempted.
- In `isUnderAttack()`, calling `goToMemberAttacked()` only when `friendUnderAttack()` is true changes the previous behavior where group movement could be triggered even when dealing with your own attacker; it’s worth double-checking that this priority change matches the desired defense behavior.
## Individual Comments
### Comment 1
<location path="src/main/java/com/deeme/behaviours/defense/DefenseMode.java" line_range="113-117" />
<code_context>
- if (botApi.getModule() != null && botApi.getModule().getClass() != DefenseModule.class
- && !((botApi.getModule().getClass() == PVPModule.class
- || botApi.getModule().getClass() == SentinelModule.class)
+ Module currentModule = botApi.getModule();
+ if (currentModule != null && !(currentModule instanceof DefenseModule)
+ && !((currentModule instanceof PVPModule || currentModule instanceof SentinelModule)
&& heroapi.isAttacking())
- && !((botApi.getModule() instanceof TemporalModule)
- && botApi.getModule().getClass() != MapModule.class)
+ && !((currentModule instanceof TemporalModule)
+ && !(currentModule instanceof MapModule))
&& isUnderAttack()) {
</code_context>
<issue_to_address>
**question (bug_risk):** Using instanceof in the Temporal/Map module condition subtly changes behavior for MapModule subclasses.
The old logic used `getClass() != MapModule.class`, while the new logic uses `!(currentModule instanceof MapModule)`. With this change, any `MapModule` subclass is now excluded from the Temporal branch, whereas previously subclasses were treated as generic `TemporalModule`s. If you have custom `MapModule` subclasses, this can change when `DefenseMode` triggers. If that’s not intended, consider preserving the exact-class check or explicitly documenting this behavioral change.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/deeme/behaviours/defense/DefenseMode.java" line_range="136-145" />
<code_context>
}
private boolean isUnderAttack() {
- goToMemberAttacked();
+ if (hasPreviousTarget() || hasAttacker()) {
+ return true;
+ }
- return hasPreviusTarget() || hasAttacker() || friendUnderAttack();
+ boolean friendNeedsHelp = friendUnderAttack();
+ if (friendNeedsHelp) {
+ goToMemberAttacked();
+ }
+ return friendNeedsHelp;
}
</code_context>
<issue_to_address>
**question (bug_risk):** The new isUnderAttack flow changes when group movement is triggered compared to the previous version.
Previously, `goToMemberAttacked()` was always called at the start of `isUnderAttack()`, regardless of the results of `hasPreviusTarget()` / `hasAttacker()` / `friendUnderAttack()`. Now it is only called when `friendUnderAttack()` is true and only after ruling out `hasPreviousTarget()` / `hasAttacker()`. As a result, if the hero has a previous target or direct attacker, it will no longer move toward attacked group members. Please confirm this behavioral change is intentional; if not, you may need to reintroduce group movement in those cases.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/deeme/behaviours/defense/DefenseMode.java" line_range="224-227" />
<code_context>
+ return antiPushLogic != null && antiPushLogic.getIgnoredPlayers().contains(playerId);
+ }
+
private void goToMemberAttacked() {
- if (!defenseConfig.goToGroup) {
+ if (!defenseConfig.goToGroup
+ || !defenseConfig.respondAttacks
+ || !defenseConfig.helpList.contains(HelpList.GROUP)) {
return;
}
</code_context>
<issue_to_address>
**question (bug_risk):** Adding respondAttacks and GROUP-helpList checks changes when group navigation can happen.
`goToMemberAttacked()` now requires `respondAttacks == true` and `helpList` containing `HelpList.GROUP`, in addition to `goToGroup`. Configs that previously triggered group movement with only `goToGroup` set will stop doing so. If that behavior change isn’t explicitly desired, consider limiting `respondAttacks`/`helpList.GROUP` to targeting logic and leaving the movement gate as just `goToGroup`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Move the player filtering logic from getTarget method to the caller in friendUnderAttack. This simplifies getTarget by removing the responsibility of checking shouldHelpPlayer, making the method more focused on selecting a target from a pre-filtered list.
The previous condition incorrectly excluded TemporalModule instances that were also MapModule. Using class equality check ensures only pure TemporalModule instances are excluded when determining if under attack.
- Merge multiple conditions into a single return statement - Remove redundant friendNeedsHelp variable - Maintain same behavior while improving readability
Update the plugin version in the manifest file to reflect the new development release.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Refine defense mode behavior to be more robust when deciding when and whom to defend.
Bug Fixes:
Enhancements: