Refactoring MustFightBattle

[Trevan]

While working on the battle tree calculator, I spent a lot of time in MustFightBattle and trying to understand its inner workings. That class is huge (>2000 lines, >70 functions) and has a lot of complicated and confusing parts (at least to a brand new developer in the code, like me).

I was wondering what if it would be ok to refactor and test pieces of MustFightBattle. Some initial ideas are:

• Step name calculation. Move this to a utility class
• Step calculation. Maybe the same utility class as step name? Or break it up into small parts. Right now, steps are calculated in 3 functions of MustFightBattle as well as FireAa and Fire.
• Rename things to fit recent terminology. There's code that references Subs and FirstStrikeUnits. I believe that it used to only support Subs but now you can create FirstStrike units that behave like subs.

Is that ok?

[LaFayette]

Moved to development.

In short @Trevan , yes, would be welcome. MustFightBattle has some of the worst metrics for technical debt, so it's a great area to fix up.

Any such updates need to be done with care though as the code is complex and could easily cause an unnoticed regression. Using tests to help with that move and validate the logic would be excellent. We should perhaps discuss a test strategy so that not everything needs to be tested by setting up a game data object but instead decomposed to use mockable strategy objects.

At the end of the day, still breaking it u and using a package structure to cluster the behavior would be excellent.

[Trevan]

@LaFayette

I've started work on the step names (MustFightBattle#determineStepStrings). I've pulled out the code in the function to a utility class (BattleSteps). Any shared code that is used by the new utility and MustFightBattle have been converted to static methods in two other utility classes. No other changes have been done to the code except to facility the conversion to static methods.

I've then started to add unittests for every possible situation in determineStepStrings. I've been using mock objects using @Mock similar to what I found in BattleTrackerTest.

Once I've finished with the unittesting, would that be a good place to create a PR and get it merged? Or should I then start refactoring the code and create a PR afterwards? I know that without refactoring, codeclimate is going to complain a lot because the functions are long and complex.

[Trevan]

Here's my vision on the battle steps.

Currently, MustFightBattle (and the other battle classes) has two sets of "steps". It has the step strings (defined at the beginning of each round in determineStepStrings) and the battle executor steps (defined in getBattleExecutables). Both of those sets are extremely similar. The code to build those two sets is different but uses a lot of common functions.

I'm thinking of creating a set of BattleStep classes. Each class would encompass the step name(s), the step executable, and the step conditional. At the beginning of a battle round, the possible list of BattleSteps would be built. This logic would use the step conditional as well as knowledge on how to order steps (such as sub ordering). determineStepStrings would then use that list to build out the list of step names. This step list would also be used by getBattleExecutables. When each step executes, it would check its conditional to see if it is still valid to use (such as all the air transports being shot down during the AA phase).

I propose doing this in 4 parts:

• Unit test determineStepStrings
• Unit test getBattleExecutables
• Unify the conditionals for all of the steps
• Move the steps into BattleStep classes

The first and second parts will be large PRs but mostly will consist of new unittests. There should be no functional changes to the codebase.
The third and fourth parts can be done in mini PRs for each of the steps.

Thoughts?

[Trevan]

I've created the PR that just creates the BattleSteps utility class and adds unittests for every possible situation.

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

The unittest file is over 2000 lines long! Since I didn't do any refactoring, I didn't try and combine tests where the only difference is attacking vs defending. So there are tests for the attacking side and tests for the defending side. In some cases, they are extremely similar; in others, they aren't.

[Trevan]

I'm working on adding unit tests for getBattleExecutables. I gave all of the executable steps an actual class name similar to how the Sub executables have class names. This allows me to check if a step is added and to execute it individually.

I've pushed my initial attempt and you can see it at https://github.com/trevan/triplea/commit/629b1d7302026695b4ab2771152fae01aee331ed.

I'm needing some help in simplifying the mock setups and in determining if new things are added to the stack.

For the mock setups, Mockito defaults to a strict mode where you can only have the stubs that will be used. Extra stubs are flagged as an error. This makes it hard to have a common stubbing since each test could be really unique. I tried to use the newDelegateBridge to create a mocked bridge but it adds more stubs than my test uses. So I had to copy the parts unique to my tests. I could switch the tests to the lenient mode but I worry about the repercussion of that.

For the stack detection, I've thought of two things:

• Stop using the instance's stack and instead use the stack that is passed into the executable. This would allow me to mock the stack and verify calls to it.
• Expose the stack to tests and add functions to grab the number of items in its deque.

Thoughts?

0 trevan committed to trevan/triplea

Give all MustFightBattle's steps a unique class for testing

Add some initial tests to show the classes being used

[LaFayette]

@Trevan Feel free to post a draft PR with work-in-progress and add a comment to the places that could use help with.

Overall, incremental and early PRs are the ticket. Early feedback and bite-sized pieces are the golden ticket. In part what you are doing is relatively difficult, if not just to do it well and get it cleanly reviewed - so thank you for the effort and hang-in there.

[Trevan]

@LaFayette Ok. I created a draft PR https://github.com/triplea-game/triplea/pull/6551

[RoiEX]

@Trevan I have a weird weekly schedule right now, but I just wanted to let you know I appreciate the work and effort you put into all of this, even if I don't have the time to review it in the detail it deserves

[Trevan]

I've created a PR that starts moving code into individual battle steps. Each part of a battle can have a name that shows up in the battle dialog and an executable. I'd like some feedback on the structure of classes. The PR, unfortunately, is based off another PR so it has a bunch of commits that are irrelevant to the individual battle steps. To see the relevant changes, you want to view changes starting with commit a7a3a385b4a6b64c29235b2a13c91e3d9c7620cf. This link should show you those: https://github.com/triplea-game/triplea/pull/6593/files/52a6b3841ac0d9f9ddb144af1b01660b635281bd..546c4a2f638fddbac3a4335eb1a955bc3c843385.

One aspect I'm not liking right now is how the IExecutable is managed. I want to abstract the call to valid so that it can be done once at the parent class but this has to be done inside of the IExecutable#execute function. So, the parent BattleStep has an abstract IExecutable inner class that each child has to extend in their getExecutable. And since the new IExecutable will need the battle state (called parameters) during its execution for the valid function, each child has to specify how to construct itself.

I tried using lombok to add a toBuilder method so that I only need to use the existing object instead of having to instantiate it, but toBuilder doesn't like abstract classes. I then tried @Builder(toBuilder = true, builderMethodName = "") which didn't work because that requires lombok 1.18.8 and triplea is using 1.18.4.

I haven't yet started on the factory class to build the steps. So MustFightBattle and BattleSteps is constructing them manually. Once I have more steps done, then I'll be able to wrap the construction in a factory class that will handle the ordering and calling valid to determine if a name is needed and if the executable should be added to the stack.

trevan created this issue in triplea-game/triplea

closed Move SubmergeVsAir and AirVsNonSubs steps to individual BattleStep classes #6593

[RoiEX]

@Trevan
IExecutables are truly a pain to work with, they are nesten in each other and are way to complicated to understand just by looking at them.
Feel free to bump the lombok version if that helps. I was surprised that it wasn't the lates version because we have a bot that regularily bumps dependencies until I realized that this dependency is managed by a lombok plugin, that's why it wasn't picked up.

@LaFayette This could be an argument to remove the lombok plugin and just declare the dependencies directly as described here: https://projectlombok.org/setup/gradle

[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.

trevan created this issue in triplea-game/triplea

closed Move SubmergeVsAir and AirVsNonSubs steps to individual BattleStep classes #6593

trevan created this issue in triplea-game/triplea

closed Move SubmergeVsAir and AirVsNonSubs steps to individual BattleStep classes #6593

[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:

1. Leave them as part of BattleState
2. Create a new object that is instantiated and passed in to the steps
3. 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