hasResource to query resources #12864
Conversation
| @@ -63,6 +64,8 @@ public class RulesAttachment extends AbstractPlayerRulesAttachment { | |||
| // condition for being at war | |||
| private @Nullable Set<GamePlayer> atWarPlayers = null; | |||
| private int atWarCount = -1; | |||
| // condition for checking resources | |||
There was a problem hiding this comment.
"condition for checking resources" - How is a String[] object a condition?
| @@ -507,6 +510,39 @@ private void resetIsAI() { | |||
| isAI = null; | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| public void setHasResource(final String value) throws GameParseException { | |||
| final String[] s = splitOnColon(value); | |||
There was a problem hiding this comment.
| @VisibleForTesting | ||
| public void setHasResource(final String value) throws GameParseException { | ||
| final String[] s = splitOnColon(value); | ||
| final int n = getInt(s[0]); |
There was a problem hiding this comment.
| throw new GameParseException("hasResource must be a positive number" + thisErrorMsg()); | ||
| } | ||
| if (s.length == 1) { | ||
| hasResource = new String[] {value, "PUs"}; |
There was a problem hiding this comment.
Maybe the string "PUs" should be extracted into a constant? (e.g. @NonNls public static final String DEFAULT_RESOURCE_UNIT)
| // validate that this resource exists in the xml | ||
| final Resource r = getData().getResourceList().getResource(s[i]); | ||
| if (r == null) { | ||
| throw new GameParseException("No resource called: " + s[i] + thisErrorMsg()); |
There was a problem hiding this comment.
Method thisErrorMsg seems odd as name (a generic error message) and usage (added as part of an error message) seem to contradict each other.
Maybe variable s[i] should be put in quotes?
| @@ -1100,6 +1152,12 @@ public void validate(final GameState data) { | |||
| this::resetAtWarPlayers); | |||
| case "atWarCount": | |||
| return MutableProperty.ofReadOnly(this::getAtWarCount); | |||
| case "haveResources": | |||
There was a problem hiding this comment.
Add annotation @NoneNls to parameter propertyName.
We want to introduce internationalization and this can help identify the strings that need translation (or as in this case that do not need it).
| @Test | ||
| void testHasResourceWithOnePlayer() throws GameParseException { | ||
| final List<GamePlayer> players1 = List.of(italians); | ||
| /* italian starts with 16 PUs, 50 Fuel and 20 Ore */ |
There was a problem hiding this comment.
Recheck if needed (seems to be a copy past comment that should not be here)
| /* italian starts with 16 PUs, 50 Fuel and 20 Ore */ | ||
| /* testing default value "PUs" for hasResource */ | ||
| attachment.setHasResource("15"); | ||
| assertTrue(attachment.checkHasResource(players1)); |
There was a problem hiding this comment.
Extract to method doing the calls for default, one resource and multiple resources) so it can be reused in method testHasResourceWithTwoPlayers()
|
|
||
| /* Testing with 1 non AI player */ | ||
| final List<GamePlayer> players1 = List.of(player1); | ||
| attachment.setIsAI("true"); |
There was a problem hiding this comment.
Extract to method doing this for a generic list so it can be reused below
| @@ -1064,6 +1104,18 @@ private boolean checkTechs(final GamePlayer player, final TechnologyFrontier tec | |||
| return found >= techCount; | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| public boolean checkHasResource(final List<GamePlayer> players) { | |||
There was a problem hiding this comment.
Maybe add JavaDoc as it is (at least for me) hard to read and understand what this method is supposed to be doing.
It seem that the purpose it to check whether the sum of the quantities of all resources from all provided players is smaller than hasResource[0].
Why is does hasResource need to be a member attribute of this class? It seems I am missing something, but then also it seems to be not clear from the code.
hasResource gives mapmakers the ability to query resources in conditions.
Format: value is the resource, can be in a delimited list. If more than one resource is listed, resources are summed. count is a number greater than 0 that resources must be equal to or greater then to be true. PUs is the default value so value="100" is the same as value="PUs" count="100". If used with players, hasResource is checked against the sum of all resources.
Ref: 2.6 Resources to be tested by conditions #12642
Testing used victorytest because it already contained references to resources.
Cheers...