Add triggers to resource income #3236
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3236 +/- ##
============================================
+ Coverage 21.01% 21.47% +0.46%
- Complexity 5672 5803 +131
============================================
Files 830 830
Lines 72732 72749 +17
Branches 11862 11863 +1
============================================
+ Hits 15282 15622 +340
+ Misses 55426 55031 -395
- Partials 2024 2096 +72
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Sorry, I'm not going to have a chance to test this tonight. If it's not merged by tomorrow afternoon, I should have time to do it then.
However, the unit test coverage actually looks pretty good. I know Codecov only says there's ~26% coverage, but after running the new unit test from the IDE, it looks like most of the important stuff is hit. There's a few branches that are missed but not too bad.
|
|
||
| private static IntegerMap<Resource> triggerResourceChange(final Set<TriggerAttachment> satisfiedTriggers, | ||
| final IDelegateBridge bridge, final String beforeOrAfter, final String stepName, final boolean useUses, | ||
| final boolean testUses, final boolean testChance, final boolean testWhen, final StringBuilder strbuf) { |
There was a problem hiding this comment.
Since this method already has a lot of parameters, the new out parameter kind of gets buried. Might want to consider returning a composite type that contains both the IntegerMap and the String. Lombok should make creating such a value type pretty lightweight.
Otherwise, at least documenting that strbuf is an output parameter would be useful. Also, a description in the Javadocs of what it will contain, or just renaming the variable, would be great, as strbuf isn't very descriptive.
There was a problem hiding this comment.
I'll just rename for now. I eventually want to refactor it further so the string for end turn report is done at the end.
| */ | ||
| public static int findNationalObjectivePus(final PlayerID player, final GameData data, final IDelegateBridge bridge) { | ||
| public static IntegerMap<Resource> findNationalObjectiveAndTriggerResources(final PlayerID player, |
There was a problem hiding this comment.
This method appears to only be called from AbstractEndTurnDelegate. Can it be moved there and made private, or do you have plans for it in a forthcoming PR? (Moving it would also get rid of one more instance of that smell in AbstractEndTurnDelegate where a superclass calls a static method in a subclass.)
There was a problem hiding this comment.
Yeah, I'm leaving it there for now as its similar to the method below it and want to further refactor them.
| testNationalObjectivesAndTriggers(player, data, bridge, triggers, objectives); | ||
|
|
||
| // Find triggers value | ||
| IntegerMap<Resource> resources = new IntegerMap<>(); |
There was a problem hiding this comment.
resources could be made final with an else block below that initializes it to an empty map.
Follow up of #3035
Functional Changes
Tests