Refactor tactical action service#3506
Conversation
|
@heisenbugged Is there any reason to not simply call 'displacement' -> 'movement' instead? Seems like an unneccessary distinction |
|
@jrkd sticking with the existing terminology mostly. movement is tracked via a 'displacement map' (which is the differential) so it makes sense to me |
|
@heisenbugged no worries. Ive been away from the code for a while so keeping current terms sounds right to me. Also, Ive made a bunch inline comments - feel free to ignore or disagree. I havent pulled it cause i dont have a local java setup anymore and better to also get someone whos currently active like Will have a go through too but saw the review come through on discord and thought i could offer a once over |
| import ti4.service.emoji.FactionEmojis; | ||
| import ti4.service.emoji.SourceEmojis; | ||
|
|
||
| public final class MoveAbilities { |
There was a problem hiding this comment.
This smells like an enum to me.
| Tile tile, | ||
| String planetName, | ||
| UnitType type, | ||
| int amt, |
There was a problem hiding this comment.
use full name for clarity (amount)
| public final class MoveAbilities { | ||
| private MoveAbilities() {} | ||
|
|
||
| public static final List<MoveAbility> ABILITIES = List.of( |
There was a problem hiding this comment.
Personally i would have these as small classes in a folder of move abilities rather than a massive list inside a single file
| } | ||
|
|
||
| public void contribute(MoveContext ctx, List<Button> buttons) { | ||
| buttons.add(Buttons.gray("exhaustAgent_belkoseaagent", "Use Belkosea Agent", FactionEmojis.belkosea)); |
There was a problem hiding this comment.
If the purpose of this is to extract game logic from discord, not using 'Button' directly will probably help - maybe something like 'Available player actions' or do this glue binding between an ability class and the buttons as a separate file away from the core ability code- appreciate this is probably out of scope
|
|
||
| public static final class Aetherstream implements MoveAbility { | ||
| public boolean enabled(MoveContext ctx) { | ||
| return ctx.player.hasTech("as") |
There was a problem hiding this comment.
Id define all these "<x ability/tech/leader id>" as constants at the top of related ability classes separete from the logic
| return planet.getUnitCountForState(unitType, owner, state) >= count; | ||
| } | ||
|
|
||
| public String buildLandButtonId(int count, UnitType unitType, Player owner) { |
There was a problem hiding this comment.
another mention of button - this is different though cause the id is still valid regardless of discord/web/whatever - perhaps this could be called something more generic like InteractionId?
| void contribute(LandingContext ctx, List<Button> buttons); | ||
| } | ||
|
|
||
| public static final List<PlanetButtonAbility> ABILITIES = List.of(new DihmohnAgent(), new TnelisDeploy()); |
There was a problem hiding this comment.
Are we able to DI these ability sets rather than a const list?
| public static final class ShroudOfLith implements PostMovementButtonAbility { | ||
| public boolean enabled(PostMovementButtonContext ctx) { | ||
| return ctx.player.hasAbility("shroud_of_lith") | ||
| && ButtonHelperFactionSpecific.getKolleccReleaseButtons(ctx.player, ctx.game) |
There was a problem hiding this comment.
This logic is particularly entangled with discord buttons - as whether you can do it is based on the current number of buttons available - perhaps theres another approach here that doesnt require checking on button state?
| return ctx.player.hasLeaderUnlocked("muaathero") | ||
| && !ctx.tile.isMecatol() | ||
| && !ctx.tile.isHomeSystem(ctx.game) | ||
| && ButtonHelper.getTilesOfPlayersSpecificUnits(ctx.game, ctx.player, UnitType.Warsun) |
There was a problem hiding this comment.
Similar to line 84 - this ability logic is tied directly to the button helper defining who owns tiles with warsuns - perhaps this could be refactored?
|
@jrkd On DI On abilities having button code On ability refactoring This is "round 1" and there are several rounds to come, continuing to separate more. The Web API will need to trigger discord buttons as it will only handle the movement part of a tactical action, so exactly what that boundary looks like I leave to future implementation thank you so much! |
3 participants
This refactors tactical actions to have colocated responsibilities
The core tactical action code is moved to
TacticalActionDisplacementService.. which does all things around handling the displacement map representing movementAll the "button assembly" code to let users trigger their abilities have been grouped into three categories
movement abilities-- things you trigger during general movementpost movement abilities-- things you trigger once your movement is done from a systemplanet abilities(few) -- things you trigger on planetsthis follows a similar pattern to the agenda resolver, using a registry instead of just interminable if/else with shared interfaces