Conversation
…ditions logic - Add try-catch block in PVPModule's onTickModule to prevent crashes - Refactor useKeyWithConditions in ShipAttacker for better readability and conditions handling - Expand healthEnemyRange default values in ExtraKeyConditions
Contributor
Reviewer's GuideAdds exception handling around PVPModule.onTickModule to prevent crashes, refactors ShipAttacker.useKeyWithConditions for clearer, more robust condition evaluation using PluginAPI and configurable conditions, and broadens the default enemy health range in ExtraKeyConditions so extra key logic can apply across the full HP spectrum. Sequence diagram for PVPModule onTickModule with exception handlingsequenceDiagram
participant PVPModule as PVPModule
participant pet as pet
participant safety as safety
participant antiPushLogic as antiPushLogic
participant shipAttacker as shipAttacker
PVPModule->>PVPModule: onTickModule()
activate PVPModule
PVPModule->>pet: setEnabled(true)
alt movementDisabledOrSafeAndOnCorrectMap
PVPModule->>PVPModule: hasTarget()
alt hasTarget
PVPModule->>PVPModule: attackLogic()
else noTarget
PVPModule->>PVPModule: set target to null
PVPModule->>shipAttacker: resetDefenseData()
PVPModule->>PVPModule: roamingLogic()
end
end
PVPModule->>antiPushLogic: registerTarget(target)
opt exceptionThrown
PVPModule->>PVPModule: catch Exception
PVPModule->>PVPModule: set target to null
end
deactivate PVPModule
Sequence diagram for ShipAttacker useKeyWithConditions decision flowsequenceDiagram
participant shipAttacker as shipAttacker
participant extra as extra
participant api as api
participant heroapi as heroapi
participant target as target
participant conditionsManagement as conditionsManagement
participant items as items
shipAttacker->>shipAttacker: useKeyWithConditions(extra, selectableItem)
alt extraDisabled
shipAttacker-->>shipAttacker: return false
else extraEnabled
shipAttacker->>shipAttacker: check keyDelay >= 500ms
alt recentKeyUse
shipAttacker-->>shipAttacker: return false
end
alt selectableItemNullAndExtraKeyNotNull
shipAttacker->>items: getItem(extra.key)
items-->>shipAttacker: selectableItem
end
alt selectableItemStillNull
shipAttacker-->>shipAttacker: return false
end
alt extraConditionNotNull
shipAttacker->>extra: condition.get(api)
extra-->>shipAttacker: conditionResult
alt conditionDisallows
shipAttacker-->>shipAttacker: return false
end
end
shipAttacker->>heroapi: getHealth().hpPercent()
alt heroHealthOutsideRange
shipAttacker-->>shipAttacker: return false
end
shipAttacker->>heroapi: getLocalTarget()
heroapi-->>shipAttacker: target
alt targetNotNull
shipAttacker->>target: getHealth().hpPercent()
alt enemyHealthOutsideRange
shipAttacker-->>shipAttacker: return false
end
end
shipAttacker->>conditionsManagement: useSelectableReadyWhenReady(selectableItem)
conditionsManagement-->>shipAttacker: used
alt selectableUsed
shipAttacker->>shipAttacker: update keyDelay
shipAttacker-->>shipAttacker: return true
else notUsed
shipAttacker-->>shipAttacker: return false
end
end
Class diagram for updated ShipAttacker, PVPModule, and ExtraKeyConditionsclassDiagram
class ShipAttacker {
-HeroAPI heroapi
-MovementAPI movement
-ConfigAPI configAPI
-GroupAPI group
-PluginAPI api
-SettingsProxy settingsProxy
-Collection~Player~ allShips
-Collection~Portal~ allPortals
-AmmoConfig ammoConfig
-Items items
-ConditionsManagement conditionsManagement
-Humanizer humanizerConfig
-char attackLaserKey
-long keyDelay
+ShipAttacker(PluginAPI api, AmmoConfig ammoConfig, Humanizer humanizerConfig)
+Ship getTarget()
+boolean useKeyWithConditions(ExtraKeyConditions extra, SelectableItem selectableItem)
}
class ExtraKeyConditions {
+boolean enable
+SelectableItemKey key
+PercentRange healthRange
+PercentRange healthEnemyRange
+Condition condition
}
class PVPModule {
-PluginAPI api
-HeroAPI heroapi
-PetAPI pet
-PVPConfig pvpConfig
-Safety safety
-AntiPushLogic antiPushLogic
-ExtraMovementLogic extraMovementLogic
-ExtraCChangerLogic extraConfigChangerLogic
-ShipAttacker shipAttacker
-Player target
+void setup()
+void onTickModule()
-void attackLogic()
-void roamingLogic()
-boolean hasTarget()
}
ShipAttacker --> ExtraKeyConditions
PVPModule --> ShipAttacker
PVPModule --> AntiPushLogic
File-Level Changes
Possibly linked issues
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:
- In
PVPModule.onTickModule, the broadcatch (Exception e)silently swallows all errors and only resetstarget; consider at least logging the exception or narrowing the catch so unexpected failures are visible during debugging. - The new logic in
useKeyWithConditionsallows the key to be used even whenheroapi.getLocalTarget()is null (enemy health range is effectively ignored in that case), whereas previously a local target was required; please confirm this behavior change is intentional.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PVPModule.onTickModule`, the broad `catch (Exception e)` silently swallows all errors and only resets `target`; consider at least logging the exception or narrowing the catch so unexpected failures are visible during debugging.
- The new logic in `useKeyWithConditions` allows the key to be used even when `heroapi.getLocalTarget()` is null (enemy health range is effectively ignored in that case), whereas previously a local target was required; please confirm this behavior change is intentional.
## Individual Comments
### Comment 1
<location> `src/main/java/com/deeme/modules/PVPModule.java:146-155` </location>
<code_context>
- target = null;
- shipAttacker.resetDefenseData();
- roamingLogic();
+ try {
+ pet.setEnabled(true);
+ if (!pvpConfig.move || (safety.tick() && checkMap())) {
+ if (hasTarget()) {
+ attackLogic();
+ } else {
+ target = null;
+ shipAttacker.resetDefenseData();
+ roamingLogic();
+ }
}
+ antiPushLogic.registerTarget(target);
+ } catch (Exception e) {
+ target = null;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching raw `Exception` without any logging may hide real issues and make debugging difficult.
Wrapping the entire `onTickModule` in a broad `catch (Exception)` and only resetting `target` will silently hide any runtime errors in movement/attack logic. Please either log the exception (via your existing logger or `api`) so recurring failures are visible, or narrow the catch to specific, expected exception types for this loop.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/deeme/modules/PVPModule.java:146` </location>
<code_context>
- target = null;
- shipAttacker.resetDefenseData();
- roamingLogic();
+ try {
+ pet.setEnabled(true);
+ if (!pvpConfig.move || (safety.tick() && checkMap())) {
</code_context>
<issue_to_address>
**issue (review_instructions):** Catching the generic Exception in onTickModule without any logging or handling beyond resetting target reduces code cleanliness and makes diagnosing issues difficult.
Wrapping the whole onTickModule body in a broad `try { ... } catch (Exception e) { target = null; }` silently swallows all unexpected errors.
For a clean and maintainable plugin, it’s better to either:
- Catch specific expected exceptions, or
- At least log the exception via Darkbot’s logging facilities so issues can be detected and debugged.
As written, any bug in `attackLogic`, `roamingLogic`, or `antiPushLogic.registerTarget` will be hidden, which works against clean, diagnosable code in a plugin context.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*.java`
**Instructions:**
It is a plugin for the darkbot bot in java, check that the code is clean and efficient.
</details>
</issue_to_address>
### Comment 3
<location> `src/main/java/com/deeme/modules/PVPModule.java:158` </location>
<code_context>
+ }
}
+ antiPushLogic.registerTarget(target);
+ } catch (Exception e) {
+ target = null;
}
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The catch block only nulls the target and does not log or handle the underlying error, which is not clean error handling for a plugin.
Inside this `catch (Exception e)` you only set `target = null;` and then ignore the exception. For a Darkbot plugin, this makes runtime failures hard to discover and can leave the bot in an inconsistent state while appearing to work.
Please consider logging the exception (or rethrowing/propagating it as appropriate), or narrowing the catch to specific expected failures and handling them explicitly.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*.java`
**Instructions:**
It is a plugin for the darkbot bot in java, check that the code is clean and efficient.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
#217
Summary by Sourcery
Improve PVP combat module robustness and refine extra key condition handling during ship attacks.
Bug Fixes:
Enhancements: