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?
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.
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
MustFightBattlehave 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
@Mocksimilar to what I found in
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,
codeclimateis going to complain a lot because the functions are long and complex.
Here's my vision on the battle steps.
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
BattleStepclasses. 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).
determineStepStringswould 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
- Unit test
- Unify the conditionals for all of the steps
- Move the steps into
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.
- Unit test
I've created the PR that just creates the
BattleStepsutility class and adds unittests for every possible situation.
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.
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 modewhere 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
newDelegateBridgeto 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 modebut 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.
@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 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
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
validso that it can be done once at the parent class but this has to be done inside of the
IExecutable#executefunction. So, the parent BattleStep has an abstract IExecutable inner class that each child has to extend in their
getExecutable. And since the new
IExecutablewill need the battle state (called
parameters) during its execution for the
validfunction, each child has to specify how to construct itself.
I tried using lombok to add a
toBuildermethod so that I only need to use the existing object instead of having to instantiate it, but
toBuilderdoesn'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
validto determine if a name is needed and if the executable should be added to the stack.
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.
@RoiEX I created a PR to bump up lombok's version.
I also tried out using the
Builderannotation but because BattleStep is an abstract class, it wouldn't work. I tried
SuperBuilderbut that only added the
toBuilderfunction on the child classes. I also tried
Withand 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#getStepthough I'm thinking of renaming it)
@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.
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 (
I went with the parameters because I wanted an immutable class but the current battle setup is using
MustFightBattleand 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
MustFightBattleand make it immutable.
I've already done some of this with the
BattleActionsinterface. 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
BattleStatewhile methods that need a
BridgeDelegatewould go in
A class called
BattleStatefor the parameters seems fine. What's the argument for making it mutable? Why not use a
toBuilderto pass updated copies of it as needed? It's very similar to a mutable object but instead you copy it to make changes.
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
getStepmethod which just instantiates a new version of itself with the latest parameters. An example of the
getStepcan be seen at https://github.com/triplea-game/triplea/pull/6593/files#diff-80f9b43b83d1dd93a77379cd8ca27443R45.
By just using
MustFightBattleas the battle state (which it currently is and it is mutable), I wouldn't need to have the
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.
I've made the change to use
BattleState. I've also merged in master and removed the irrelevant commits. I've removed the WIP flag on the PR.
I'm looking at converting the retreat steps and am trying to figure out what to do with
I don't think they fit on the
BattleStateinterface 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
- 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
BattleStatesince they appear to just need the data that is being encapsulated in
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
MustFightBattlehas a copy to
BattleTrackerand uses it in a few steps but I'd rather not add that to the
- Leave them as part of
@Trevan Initial reaction is to go with
1: Leave them as part of BattleState. My reasoning is if you pass
BattleStateas a parameter and the methods are static anyways, it hints that the methods are really just operations of
BattleStateand belong to battle state.
DelegateFinder.battleDelegate(gameData).getBattleTracker()is a (design-wise) dangerous way to get at battle tracker.
gameDatais 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
gameDatabeing 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.
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