Refactoring MustFightBattle

Trevan

@RoiEX I created a PR to bump up lombok's version.

https://github.com/triplea-game/triplea/pull/6597

I also tried out using the Builder annotation but because BattleStep is an abstract class, it wouldn't work. I tried SuperBuilder but that only added the toBuilder function on the child classes. I also tried With and that added an abstract method. I don't see another way to do it with lombok. I think each child class is just going to have to define their own builder method (currently called BattleStep#getStep though I'm thinking of renaming it)

Trevan

@RoiEX @LaFayette I think if triplea moved from the io.franzbecker.gradle-lombok plugin to the io.freefair.lombok, it would get regular updates. The io.franzbecker.gradle-lombok doesn't appear to control the version of lombok that it includes while io.freefair.lombok does. So when lombok is updated, io.freefair.lombok will release a new version that includes the new lombok.

Trevan

I've been thinking about the IExecutable and it needing the current parameters. What if, instead of having a separate immutable parameters class, I create an interface (call it BattleState) that the battle classes (such as MustFightBattle) implements and this exposes the various parameters (attacker, attackingUnits)?

I went with the parameters because I wanted an immutable class but the current battle setup is using MustFightBattle and updates it as it goes on. So this change would fit more in line with the existing code but would create a contract between the battle steps and the battle state. Once that contract is complete, a future refactor could separate out the the state from MustFightBattle and make it immutable.

I've already done some of this with the BattleActions interface. That interface was to start defining the contract between the steps and interaction with the rest of the system. I would just put methods that only edit the battle state in BattleState while methods that need a BridgeDelegate would go in BattleActions.

Thoughts?

LaFayette

@Trevan said in Refactoring MustFightBattle:

BattleState

A class called BattleState for the parameters seems fine. What's the argument for making it mutable? Why not use a toBuilder to pass updated copies of it as needed? It's very similar to a mutable object but instead you copy it to make changes.

Trevan

@LaFayette

The argument is about the issue with how the IExecutable is built. When the IExecutable is built, it will have a different battle state then when it is run. So in the execute, I have to recreate the step with the latest battle state and it felt clunky.

You can see the code at https://github.com/triplea-game/triplea/pull/6593/files#diff-20f9a219c282060fe55fd130579ecf06R82 (lines 82 to 87 of the BattleStep class). Each of the children steps have to implement their own getStep method which just instantiates a new version of itself with the latest parameters. An example of the getStep can be seen at https://github.com/triplea-game/triplea/pull/6593/files#diff-80f9b43b83d1dd93a77379cd8ca27443R45.

By just using MustFightBattle as the battle state (which it currently is and it is mutable), I wouldn't need to have the getStep method.

LaFayette

I think that is compelling enough of reason. I think the order of operations is to first simplify the existing code, attach testing to it, then think about next steps. Eventually we'll want to be able to serialize more easily the battle state to save games. I think to get there we'll need a way to recreate the current battle step more easily rather than have it be computed and then steps added and those added to the state that is then saved. That is perhaps confusing, but said another way, we should be able to determine the next battle step based on the game history and current state rather than relying on having a data structure be serialized and then re-loaded.

Trevan

I've made the change to use MustFightBattle as the BattleState. I've also merged in master and removed the irrelevant commits. I've removed the WIP flag on the PR.

Trevan

I'm looking at converting the retreat steps and am trying to figure out what to do with MustFightBattle#getAttackerRetreatTerritories and MustFightBattle#getEmptyOrFriendlySeaNeighbors.

I don't think they fit on the BattleState interface but they are used in multiple retreat steps. So I need to make it available to the different steps.

I can think of several options:

Leave them as part of BattleState
Create a new object that is instantiated and passed in to the steps
Create a static utility class that has these methods

If I do #2 or #3, the methods would just accept BattleState since they appear to just need the data that is being encapsulated in BattleState.

What fits better with the coding style of triplea? Or is there another way I could do?

In addition, is using DelegateFinder.battleDelegate(gameData).getBattleTracker() a currently accepted way to get the BattleTracker? MustFightBattle has a copy to BattleTracker and uses it in a few steps but I'd rather not add that to the BattleState interface either.

LaFayette

@Trevan Initial reaction is to go with 1: Leave them as part of BattleState. My reasoning is if you pass BattleState as a parameter and the methods are static anyways, it hints that the methods are really just operations of BattleState and belong to battle state.

DelegateFinder.battleDelegate(gameData).getBattleTracker() is a (design-wise) dangerous way to get at battle tracker. gameData is plumbed into too many places, seemingly everywhere, and hence we can create some bad static coupling by arriving at other objects through gameData (which in turn then has gameData being plumbed into more locations).

Fundamentally the IOC (inversion-of-control) was never applied to TripleA. It is a major weakness, makes testing very difficult, module boundaries become poorly defined and resulted in massive dependencies on GameData.

In short,DelegateFinder.battleDelegate(gameData).getBattleTracker() is perhaps an okay way to get the battle tracker, it's best that anything that makes use of it to accept the battle tracker as a parameter. When in doubt, imperative shell / functional core is the design principle I lean on: https://www.destroyallsoftware.com/screencasts/catalog/functional-core-imperative-shell

Trevan

I've been working on the offensive subs submerge/withdraw step and discovered that my idea of having a unified valid() check that works for both getNames and execute won't work.

In the offensive subs submerge/withdraw step, execute shouldn't run if there is a destroyer present. But getNames is called at the beginning of the round and there might be a destroyer present that will die before execute is called. If getNames doesn't return the submerge/withdraw step, then an error will occur.

I was actually able to trigger this error from the master branch when Submersible Subs is false, Sub Retreat Before Battle is false, and a destroyer is present at the beginning of the round but then is destroyed during the round. If Submersible Subs is true and Sub Retreat Before Battle is false, then this error doesn't occur because it doesn't check for destroyers.

I'm going to be removing valid() as a public method from the BattleStep interface. Instead, getNames and execute will do their own internal valid check. This check might be the same or different, depending on the step.