-
Notifications
You must be signed in to change notification settings - Fork 0
Add conditions to card targeting #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a conditional targeting system for cards, which is a valuable addition. The implementation is generally well-done. My review includes suggestions to improve encapsulation and robustness in TargetingInfo by using defensive copying and returning unmodifiable lists. I've also pointed out an opportunity to refactor TargetingArrow to use a more modern, stream-based approach for checking conditions and to address code duplication in the rendering logic. These changes will enhance the code's maintainability and prevent potential bugs.
| public TargetingInfo(TargetType targetType, int range, Condition... conditions) { | ||
| this.targetType = targetType; | ||
| this.range = range; | ||
| this.conditions = Arrays.asList(conditions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Arrays.asList() creates a list that is a view of the original array. If the caller modifies the conditions array after calling this constructor, it will also modify the list within this TargetingInfo instance. This can lead to unexpected behavior and subtle bugs. To ensure the TargetingInfo object is immutable with respect to its conditions, you should create a defensive copy.
| this.conditions = Arrays.asList(conditions); | |
| this.conditions = new java.util.ArrayList<>(Arrays.asList(conditions)); |
| } | ||
|
|
||
| public List<Condition> conditions() { | ||
| return conditions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning the internal list directly allows client code to modify it (if it's mutable), which breaks encapsulation. It's a good practice to return an unmodifiable view of the list to protect the internal state of the object. This is especially important if you change the constructor to create a mutable ArrayList as suggested in another comment.
| return conditions; | |
| return java.util.Collections.unmodifiableList(conditions); |
| if (info.targetType() == TargetType.HEXAGON) { | ||
| target = tile; | ||
| if (target == null) { | ||
| if (target == null || !checkConditions(info, state.world(), target)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is significant code duplication between this block for TargetType.HEXAGON and the one for TargetType.CARD_PLAYER on lines 65-81. The logic for rendering the target highlight is identical. Consider extracting this duplicated code into a private helper method to improve maintainability. The only difference is how the target is determined (tile vs tile.actor()).
| private boolean checkConditions(TargetingInfo info, nomadrealms.context.game.world.World world, Target target) { | ||
| for (Condition condition : info.conditions()) { | ||
| if (!condition.test(world, target)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be written more concisely using Java Streams. Additionally, it's better to import nomadrealms.context.game.world.World and use its simple name World in the method signature for improved readability.
After adding import nomadrealms.context.game.world.World; at the top of the file, you can simplify this method as suggested.
private boolean checkConditions(TargetingInfo info, World world, Target target) {
return info.conditions().stream().allMatch(c -> c.test(world, target));
}3000dcd to
ae95473
Compare
- Create `Condition` interface for target validation - Update `TargetingInfo` to accept a list of conditions (via varargs) - Expose `world()` in `GameState` - Update `TargetingArrow` to check conditions before highlighting a target
ae95473 to
f0ced5a
Compare
Implemented a conditional targeting system for cards. This allows
TargetingInfoto include a set ofConditions that evaluate against the game world and the potential target. TheTargetingArrowUI component now respects these conditions, preventing the selection and highlighting of invalid targets. AddedConditioninterface and updatedTargetingInfoandGameStateto support this logic.PR created automatically by Jules for task 17706898319591583784 started by @Lunkle