Fix removing disabled units with battle AA #6271
Conversation
|
Code Climate has analyzed commit f61b41e and detected 0 issues on this pull request. View more on Code Climate. |
Codecov Report
@@ Coverage Diff @@
## master #6271 +/- ##
=========================================
Coverage 23.05% 23.06%
- Complexity 6607 6608 +1
=========================================
Files 1109 1109
Lines 78350 78353 +3
Branches 11446 11446
=========================================
+ Hits 18066 18069 +3
Misses 58081 58081
Partials 2203 2203
Continue to review full report at Codecov.
|
|
@ron-murhammer it looks like the PR comment is useful and perhaps would do well to be in the commit history. I think it should be kept in mind that comments made in a PR are only for the reviewer and are of extremely transitory value. IMO ideally everything in the PR overview is auto-filled from the commit comment and a PR author then only needs to check a box and explain the testing that they have then done. |
| @@ -576,6 +576,7 @@ private static void removeFromDependents( | |||
| @Override | |||
| public void fight(final IDelegateBridge bridge) { | |||
| removeUnitsThatNoLongerExist(); | |||
There was a problem hiding this comment.
The pattern of mutating void method calls is unfortunate. The reasoning/logic behind the ordering is not captured, makes it really brittle as re-ordering lines or adding functionality can cause unexpected problems.
It's also not clear as well why the ordering is as it is, we also run into issues as well for reading the functions as the void calls you have to trace down to see what is happening. Much of this perhaps could just as well be inlined and/or made functional so the control flow is captured at a high level. Regardless, it's just kinda sad to see this anti-pattern. There are some interesting tools out there that would perhaps catch this, those tools permute lines and re-run tests to see if any file due to line permutations and then gives you a score of methods that fail highlighting brittlness.
|
I updated the squash commit message to have the overview included here. |
Addresses issue 1 here: https://forums.triplea-game.org/topic/1031/world-of-war-heroes-official-thread/110
AA rolls were done before disabled units (bombing damage greater than
maxOperationalDamage) are removed from combat. They should just be removed before the battle even starts rather than after AA rolls.