refactor: replace marker battle events with typed dispatch#49
Merged
Conversation
📝 WalkthroughWalkthroughMultiple report event classes were converted to Java records and BattleEvent was extended with a generic Visitor interface. Consumers (console, GUI dialogue, arena model) were refactored to dispatch events via the new visitor callbacks instead of runtime instanceof checks. Changes
Sequence Diagram(s)sequenceDiagram
participant E as Event (BattleEvent)
participant V as Visitor (BattleEvent.Visitor)
participant C as Consumer (Formatter / Model)
C->>E: call event.visit(visitorInstance)
E->>V: dispatch to onX(this)
V-->>C: return formatted data / perform state update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java (1)
46-80: Consider caching the formatted text to avoid double dispatch.
playbackDelayMillisvisits the event twice: once forbaseDelay(line 47) and once viaformat(event)(line 78). This is a minor inefficiency.♻️ Optional optimization
public int playbackDelayMillis(BattleEvent event) { + String formatted = format(event); int baseDelay = event.visit(new BattleEvent.Visitor<>() { // ... visitor callbacks unchanged ... }); - int typewriterDelay = format(event).length() * 12 + 520; + int typewriterDelay = formatted.length() * 12 + 520; return Math.max(baseDelay, Math.min(2_000, typewriterDelay)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java` around lines 46 - 80, Cache the formatted string once and avoid visiting the event twice: call String formatted = format(event) and compute typewriterDelay from formatted, then determine baseDelay by checking the event concrete type (use instanceof against ActionEvent, NarrationEvent, RoundStartEvent, RoundSummaryEvent, SkippedTurnEvent, StatusEffectReportEvent) instead of calling event.visit again; update playbackDelayMillis to use the single cached formatted value and the instanceof-based branch to compute the same numeric baseDelay values.src/main/java/sc2002/turnbased/report/ActionEvent.java (1)
24-27: Tighten constructor null-safety for event payload fields.Please validate
actorName,actionName,targetNamein the canonical constructor, and fail fast in the convenience constructor foractor,target, andattackResolutionso callers get clearer errors.Proposed refactor
public ActionEvent { actorId = Objects.requireNonNull(actorId, "actorId"); + actorName = Objects.requireNonNull(actorName, "actorName"); + actionName = Objects.requireNonNull(actionName, "actionName"); targetId = Objects.requireNonNull(targetId, "targetId"); + targetName = Objects.requireNonNull(targetName, "targetName"); statusEffectNotes = List.copyOf(Objects.requireNonNull(statusEffectNotes, "statusEffectNotes")); } public ActionEvent(Combatant actor, String actionName, Combatant target, AttackResolution attackResolution) { this( - actor.combatantId(), - actor.getName(), - actionName, - target.combatantId(), - target.getName(), - attackResolution.hpBefore(), - attackResolution.hpAfter(), - attackResolution.attackUsed(), - attackResolution.targetDefense(), - attackResolution.damage(), - attackResolution.targetEliminated(), - StatusEffectReportMapper.toNotes(attackResolution.statusEffectOutcomes()) + Objects.requireNonNull(actor, "actor").combatantId(), + Objects.requireNonNull(actor, "actor").getName(), + Objects.requireNonNull(actionName, "actionName"), + Objects.requireNonNull(target, "target").combatantId(), + Objects.requireNonNull(target, "target").getName(), + Objects.requireNonNull(attackResolution, "attackResolution").hpBefore(), + Objects.requireNonNull(attackResolution, "attackResolution").hpAfter(), + Objects.requireNonNull(attackResolution, "attackResolution").attackUsed(), + Objects.requireNonNull(attackResolution, "attackResolution").targetDefense(), + Objects.requireNonNull(attackResolution, "attackResolution").damage(), + Objects.requireNonNull(attackResolution, "attackResolution").targetEliminated(), + StatusEffectReportMapper.toNotes( + Objects.requireNonNull(attackResolution, "attackResolution").statusEffectOutcomes() + ) ); }Also applies to: 30-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/report/ActionEvent.java` around lines 24 - 27, The compact/canonical constructor for ActionEvent currently null-checks actorId, targetId and statusEffectNotes but misses actorName, actionName and targetName—add Objects.requireNonNull checks for actorName, actionName, and targetName and copy statusEffectNotes as before; additionally, in the convenience constructor(s) (the constructor taking Actor actor, Actor target, and AttackResolution attackResolution or similar overloads referenced around the other constructor block), perform immediate null checks for actor, target, and attackResolution (and any derived names) so the constructor fails fast with clear NPE messages rather than deferring validation—update the constructors that delegate to the canonical one to validate inputs before using actor.getId()/getName() or target.getId()/getName().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/sc2002/turnbased/report/ActionEvent.java`:
- Around line 24-27: The compact/canonical constructor for ActionEvent currently
null-checks actorId, targetId and statusEffectNotes but misses actorName,
actionName and targetName—add Objects.requireNonNull checks for actorName,
actionName, and targetName and copy statusEffectNotes as before; additionally,
in the convenience constructor(s) (the constructor taking Actor actor, Actor
target, and AttackResolution attackResolution or similar overloads referenced
around the other constructor block), perform immediate null checks for actor,
target, and attackResolution (and any derived names) so the constructor fails
fast with clear NPE messages rather than deferring validation—update the
constructors that delegate to the canonical one to validate inputs before using
actor.getId()/getName() or target.getId()/getName().
In `@src/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java`:
- Around line 46-80: Cache the formatted string once and avoid visiting the
event twice: call String formatted = format(event) and compute typewriterDelay
from formatted, then determine baseDelay by checking the event concrete type
(use instanceof against ActionEvent, NarrationEvent, RoundStartEvent,
RoundSummaryEvent, SkippedTurnEvent, StatusEffectReportEvent) instead of calling
event.visit again; update playbackDelayMillis to use the single cached formatted
value and the instanceof-based branch to compute the same numeric baseDelay
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71c9e4ef-3b98-46ee-96ce-8af56abe1967
📒 Files selected for processing (12)
src/main/java/sc2002/turnbased/report/ActionEvent.javasrc/main/java/sc2002/turnbased/report/BattleEvent.javasrc/main/java/sc2002/turnbased/report/CombatantSummary.javasrc/main/java/sc2002/turnbased/report/NarrationEvent.javasrc/main/java/sc2002/turnbased/report/RoundStartEvent.javasrc/main/java/sc2002/turnbased/report/RoundSummaryEvent.javasrc/main/java/sc2002/turnbased/report/SkippedTurnEvent.javasrc/main/java/sc2002/turnbased/report/StatusEffectReportEvent.javasrc/main/java/sc2002/turnbased/ui/BattleConsoleFormatter.javasrc/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.javasrc/main/java/sc2002/turnbased/ui/gui/view/ArenaSceneModel.javasrc/test/java/sc2002/turnbased/domain/EnemyCombatantTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java (1)
46-64: Consider using visitor pattern forplaybackDelayMillisfor consistency.The
format()method was refactored to use the visitor pattern, butplaybackDelayMillis()still usesinstanceofchecks. This creates an inconsistency: if a newBattleEventtype is added,format()will fail to compile (enforcing the update), whileplaybackDelayMillis()will silently fall back to the defaultbaseDelay = 1_000.Additionally,
format(event)is called here to computetypewriterDelay, but callers likely also callformat()separately for display, resulting in redundant formatting.♻️ Suggested refactor using visitor for delay calculation
public int playbackDelayMillis(BattleEvent event) { String formatted = format(event); int typewriterDelay = formatted.length() * 12 + 520; - int baseDelay = 1_000; - if (event instanceof ActionEvent) { - baseDelay = 1_350; - } else if (event instanceof NarrationEvent) { - baseDelay = 1_250; - } else if (event instanceof RoundStartEvent) { - baseDelay = 850; - } else if (event instanceof RoundSummaryEvent) { - baseDelay = 900; - } else if (event instanceof SkippedTurnEvent) { - baseDelay = 1_150; - } else if (event instanceof StatusEffectReportEvent) { - baseDelay = 1_100; - } + int baseDelay = event.visit(new BattleEvent.Visitor<>() { + `@Override` public Integer onAction(ActionEvent e) { return 1_350; } + `@Override` public Integer onNarration(NarrationEvent e) { return 1_250; } + `@Override` public Integer onRoundStart(RoundStartEvent e) { return 850; } + `@Override` public Integer onRoundSummary(RoundSummaryEvent e) { return 900; } + `@Override` public Integer onSkippedTurn(SkippedTurnEvent e) { return 1_150; } + `@Override` public Integer onStatusEffectReport(StatusEffectReportEvent e) { return 1_100; } + }); return Math.max(baseDelay, Math.min(2_000, typewriterDelay)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java` around lines 46 - 64, Replace the instanceof chain in playbackDelayMillis(BattleEvent) with a visitor-based dispatch: add a playback-delay method to your BattleEvent visitor (e.g., PlaybackDelayVisitor or add a getPlaybackBaseDelay(BattleEventVisitor) visit method for each concrete event) and implement it in all concrete events (ActionEvent, NarrationEvent, RoundStartEvent, RoundSummaryEvent, SkippedTurnEvent, StatusEffectReportEvent) so playbackDelayMillis calls event.accept(playbackDelayVisitor) to obtain baseDelay instead of using instanceof. Also avoid reformatting by changing playbackDelayMillis to accept the already-formatted string (or the formatted length) so it computes typewriterDelay from that input (use playbackDelayMillis(BattleEvent, String formatted) or playbackDelayMillis(BattleEvent, int formattedLength)), and update callers to pass the output of format(event) to eliminate redundant formatting; ensure all new visitor methods are implemented for every BattleEvent concrete class to keep compilation enforcement consistent with format().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java`:
- Around line 46-64: Replace the instanceof chain in
playbackDelayMillis(BattleEvent) with a visitor-based dispatch: add a
playback-delay method to your BattleEvent visitor (e.g., PlaybackDelayVisitor or
add a getPlaybackBaseDelay(BattleEventVisitor) visit method for each concrete
event) and implement it in all concrete events (ActionEvent, NarrationEvent,
RoundStartEvent, RoundSummaryEvent, SkippedTurnEvent, StatusEffectReportEvent)
so playbackDelayMillis calls event.accept(playbackDelayVisitor) to obtain
baseDelay instead of using instanceof. Also avoid reformatting by changing
playbackDelayMillis to accept the already-formatted string (or the formatted
length) so it computes typewriterDelay from that input (use
playbackDelayMillis(BattleEvent, String formatted) or
playbackDelayMillis(BattleEvent, int formattedLength)), and update callers to
pass the output of format(event) to eliminate redundant formatting; ensure all
new visitor methods are implemented for every BattleEvent concrete class to keep
compilation enforcement consistent with format().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78222dfa-f69a-4461-aceb-57bd82c397cd
📒 Files selected for processing (2)
src/main/java/sc2002/turnbased/report/ActionEvent.javasrc/main/java/sc2002/turnbased/ui/gui/playback/BattleDialogueFormatter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/sc2002/turnbased/report/ActionEvent.java
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.
Closes #50
Summary
Summary by CodeRabbit