Add new resource attribute isDisplayFor #3015

ron-murhammer · 2018-02-17T21:09:45Z

Addresses portion of https://forums.triplea-game.org/topic/558/fuel-enhancements

Functional Changes

Added new isDisplayFor property for resources which can be "NONE" or list of players (defaults to showing for all players). Primarily used to display/hide certain resources for specific nations. Follows up the changes in Add Player Resource Bar #3013 .

Example

  <resourceList>
    <resource name="techTokens" isDisplayedFor="NONE"/>
    <resource name="PUs" isDisplayedFor="Union"/>
    <resource name="Manpower" isDisplayedFor="Confederate"/>
    <resource name="Supplies" isDisplayedFor="Union:Confederate"/>
    <resource name="Industry"/>
    <resource name="Leadership"/>
  </resourceList>

codecov-io · 2018-02-17T21:16:18Z

Codecov Report

Merging #3015 into master will increase coverage by <.01%.
The diff coverage is 25.8%.

@@             Coverage Diff              @@
##             master    #3015      +/-   ##
============================================
+ Coverage     20.56%   20.56%   +<.01%     
- Complexity     5822     5823       +1     
============================================
  Files           819      819              
  Lines         72917    72937      +20     
  Branches      12011    12015       +4     
============================================
+ Hits          14995    15001       +6     
- Misses        55830    55844      +14     
  Partials       2092     2092

Impacted Files	Coverage Δ	Complexity Δ
...in/java/games/strategy/triplea/ui/ResourceBar.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...in/java/games/strategy/engine/data/GameParser.java	`69.43% <33.33%> (-0.84%)`	`175 <2> (-1)`
...main/java/games/strategy/engine/data/Resource.java	`83.33% <75%> (-16.67%)`	`2 <1> (+1)`
src/main/java/games/strategy/net/nio/Decoder.java	`72.82% <0%> (-2.18%)`	`24% <0%> (-1%)`
...gy/engine/lobby/server/db/AccessLogController.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...engine/lobby/server/login/LobbyLoginValidator.java	`52.02% <0%> (ø)`	`29% <0%> (ø)`	⬇️
.../engine/lobby/server/login/CompositeAccessLog.java	`81.81% <0%> (ø)`	`4% <0%> (ø)`	⬇️
.../java/games/strategy/engine/data/DefaultNamed.java	`66.66% <0%> (+22.22%)`	`7% <0%> (+2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a03477...d50edba. Read the comment docs.

DanVanAtta

Good feature.
The updates could really use a simplification pass, I left some suggestions.
The example in overview is good, can we commit that somewhere to codebase, the docs, a README, or tests so that it is not lost?

DanVanAtta · 2018-02-17T23:48:13Z

src/main/java/games/strategy/engine/data/Resource.java

+  }
+
+  public boolean isDisplayedFor(final PlayerID player) {
+    return players != null ? players.contains(player) : true;


Should be able to remove this null check, does not look that players would never be null.

I believe it can be null when deserializing an old save game where Resource#players does not exist. The alternative is to initialize the field to an empty list in a readObject() method.

If we're going to leave it nullable, then I'd recommend we add a TODO that says the null check can be removed in the next incompatible release, similar to what we've done in other domain classes when we've introduced this scenario.

EDIT: If players stays nullable, we should be able to get rid of the conditional operator:

return players == null || players.contains(player);

Correct. Null check is needed for old save games. I'll add a TODO to remove.

DanVanAtta · 2018-02-17T23:49:11Z

src/main/java/games/strategy/engine/data/Resource.java

@@ -14,4 +19,14 @@
  public Resource(final String name, final GameData data) {
    super(name, data);


nit: Would you mind to update this constructor to do a this call and pass the new ArrayLIst<>() default value for players? This way we have only one constructor doing any real work. In this way too the players field can be made final.

DanVanAtta · 2018-02-17T23:53:41Z

src/main/java/games/strategy/triplea/ui/ResourceBar.java

+        for (int i = 0; i < resourceStats.size(); i++) {
+          final IStat resourceStat = resourceStats.get(i);
+          final Resource resource = gameData.getResourceList().getResource(resourceStat.getName());
+          final String quantity = resourceStat.getFormatter().format(resourceStat.getValue(player, gameData));


You would think that this could be as simple as:

resourceStat.format(player, gameData);

There is some extra complexity in this block. I recommend adding some commentary, or even better making it a bit more self documenting and extracting to well named helper methods. Ultimately I think some API updates are in order, but I could agree to that being saved for another day.

Agree it is a bit long but didn't want to update the API for IStat at this point.

DanVanAtta · 2018-02-18T00:04:55Z

src/main/java/games/strategy/engine/data/GameParser.java

+      final String name = element.getAttribute("name");
+      final String isDisplayedFor = element.getAttribute("isDisplayedFor");
+      final List<PlayerID> players = data.getPlayerList().getPlayers();
+      if (isDisplayedFor.equalsIgnoreCase("NONE")) {


nit, magic constant "NONE". Looking at the rest of this update, it looks to be the default missing value supplied by XML parser. If someone were looking at this code without looking at this PR, that may be very unclear where NONE is coming from and whether it was user supplied in the XML. A well named constant could help avoid that confusion.

Not sure a constant at the top of a huge class is any better. Really the parser class needs chunked up and then a constant would help more.

Added constant.

DanVanAtta · 2018-02-18T00:10:26Z

src/main/java/games/strategy/engine/data/GameParser.java

+    for (final Element element : getChildren("resource", root)) {
+      final String name = element.getAttribute("name");
+      final String isDisplayedFor = element.getAttribute("isDisplayedFor");
+      final List<PlayerID> players = data.getPlayerList().getPlayers();


nit: unnecessary data access here. This is only needed final List<PlayerID> players = data.getPlayerList().getPlayers(); if the boolean conditional is false.

This block could be made a bit simpler. I see a problem that players is initialized before it is needed. Thinking about flow, we grab XML isDisplayedData, we grab players list from game data, then we check isDisplayedData and if none then we clear the player list we just grabbed : )

So to be specific the two problems I see are:

players variable is initialized before it is needed, outside of the scope of if block

players variable is re-used for a second purpose (SRP violation). That would be fixed by removing the players.clear calls and simply creating new lists as needed.

Combining the above with some other suggestions I had along the way, this block can be simplified to at least the following:

if(element.getAttribute("name") .getAttribute("isDisplayedFor") .equalsIgnoreCase("NONE")) { data.getResourceList().addResource(new Resource(name, data)); } else { final List<PlayerID> players = data.getPlayerList().getPlayers(); data.getResourceList().addResource(new Resource( name, data, isDisplayedFor.split(":").stream() .map(s -> data.getPlayerList().getPlayerId(s)) .collect(Collectors.toList()); }

Personally, I'd like to see it go a bit further even, that such logic is moved and fully contained to a new class (strategy pattern). In this way we could test the new logic much more readily, even if we can't yet test all of GameParser.

I really think that is much harder to read and maintain than the original.

The original, it's hard to tell where final List<PlayerID> players = data.getPlayerList().getPlayers(); is used. In the updated version it's clear it's in the else block. Another iteration can probably go further and do it all as one block. Regardless, there is the concern that the original and updated are hard to maintain and lacking tests.

These are still problems if you sue streaming notation or not:

- players variable is initialized before it is needed, outside of the scope of if block - players variable is re-used for a second purpose (SRP violation). That would be fixed by removing the players.clear calls and simply creating new lists as needed.

+ if (isDisplayedFor.equalsIgnoreCase(RESOURCE_IS_DISPLAY_FOR_NONE)) { data.getResourceList().addResource(new Resource(name, data)); + } else if (!isDisplayedFor.isEmpty()) { List<PlayerId> playersWithResource = new ArrayList<>(); + for (final String s : isDisplayedFor.split(":")) { + final PlayerID player = data.getPlayerList().getPlayerId(s); + if (player == null) { + throw new GameParseException("Parse resources could not find player: " + s); + } + playersWithResource.add(player); + } data.getResourceList().addResource(new Resource(name, data, players)); + } + }

That woudl fix SRP and make it a bit less direct. The single void call at the end I don't think is a good thing, it requires tracing the 'players' object through the above if/else/for block, it's not very direct.

I can see what you are getting at just isn't that easy to make it much simpler. I refactored it to instead not re-use players list across if conditions. I'd say its about the same in complexity/readability but it essentially comes down to if avoiding the re-use and clearing of players list is worth having an extra if and 3 calls instead of 1 to addResource. Its probably more explicit this way.

ssoloff · 2018-02-18T00:56:12Z

src/main/java/games/strategy/engine/data/Resource.java

+
+  public Resource(final String name, final GameData data, final List<PlayerID> players) {
+    this(name, data);
+    this.players = players;


We probably should be making a defensive copy of the incoming list.

So I think most of the data objects make a copy if they have a getter to return a copy. I'll have to look to see what similar classes do when taking in a list.

ssoloff · 2018-02-18T01:13:17Z

src/main/java/games/strategy/triplea/ui/ResourceBar.java

-        for (int i = 0; i < resources.size(); i++) {
-          final String quantity = resources.get(i).getFormatter().format(resources.get(i).getValue(player, gameData));
+        for (int i = 0; i < resourceStats.size(); i++) {
+          final IStat resourceStat = resourceStats.get(i);


Might as well extract a new local for the JLabel while you're doing this for resourceStat to avoid the repeated calls to labels.get(i) below.

final JLabel label = labels.get(i);

ssoloff · 2018-02-18T01:24:25Z

src/main/java/games/strategy/triplea/ui/ResourceBar.java

-          final String quantity = resources.get(i).getFormatter().format(resources.get(i).getValue(player, gameData));
+        for (int i = 0; i < resourceStats.size(); i++) {
+          final IStat resourceStat = resourceStats.get(i);
+          final Resource resource = gameData.getResourceList().getResource(resourceStat.getName());


One simplification here may be to use ResourceStat#resource rather than having to do the resource lookup again:

diff --git a/src/main/java/games/strategy/triplea/ui/ResourceBar.java b/src/main/java/games/strategy/triplea/ui/ResourceBar.java index 38dcf4535..32ad37a92 100644 --- a/src/main/java/games/strategy/triplea/ui/ResourceBar.java +++ b/src/main/java/games/strategy/triplea/ui/ResourceBar.java @@ -12,7 +12,6 @@ import games.strategy.engine.data.GameData; import games.strategy.engine.data.PlayerID; import games.strategy.engine.data.Resource; import games.strategy.engine.data.events.GameDataChangeListener; -import games.strategy.engine.stats.IStat; import games.strategy.triplea.Constants; /** @@ -22,7 +21,7 @@ public class ResourceBar extends AbstractStatPanel implements GameDataChangeList private static final long serialVersionUID = -7713792841831042952L; private final UiContext uiContext; - private final List<IStat> resourceStats = new ArrayList<>(); + private final List<ResourceStat> resourceStats = new ArrayList<>(); private final List<JLabel> labels = new ArrayList<>(); public ResourceBar(final GameData data, final UiContext uiContext) { @@ -65,8 +64,8 @@ public class ResourceBar extends AbstractStatPanel implements GameDataChangeList final PlayerID player = gameData.getSequence().getStep().getPlayerId(); if (player != null) { for (int i = 0; i < resourceStats.size(); i++) { - final IStat resourceStat = resourceStats.get(i); - final Resource resource = gameData.getResourceList().getResource(resourceStat.getName()); + final ResourceStat resourceStat = resourceStats.get(i); + final Resource resource = resourceStat.resource; final String quantity = resourceStat.getFormatter().format(resourceStat.getValue(player, gameData)); labels.get(i).setVisible(resource.isDisplayedFor(player)); try {

DanVanAtta · 2018-02-18T01:34:58Z

#3016 would resolve conflicts in this PR.

ron-murhammer · 2018-02-18T02:13:37Z

@ssoloff @DanVanAtta Added commit to address comments.

Planning to update POS2 XML with example after this is merged.

ssoloff · 2018-02-18T03:14:03Z

src/main/java/games/strategy/engine/data/Resource.java

 public class Resource extends NamedAttachable {
  private static final long serialVersionUID = 7471431759007499935L;

+  private List<PlayerID> players = new ArrayList<>();


I think this field can now be final if you remove the initialization.

Good point. Updated.

ron-murhammer · 2018-02-18T04:36:07Z

@DanVanAtta Comments should be addressed now.

DanVanAtta

Thanks for following up @ron-murhammer . Looking at the latest update, while we do have some additional branching going on, the flow is more linear, it's easier to understand. What I mean by linear is you don't really have to backtrack to remind yourself what the players variable is, whether it's safe to clear it, checking out interactions, etc... The code can be pretty much read in one pass now, that is good - thank you.

DanVanAtta · 2018-02-18T19:29:37Z

src/main/java/games/strategy/engine/data/GameParser.java

-        players.clear();
+      if (isDisplayedFor.isEmpty()) {
+        data.getResourceList().addResource(new Resource(name, data, data.getPlayerList().getPlayers()));
+      } else if (isDisplayedFor.equalsIgnoreCase(RESOURCE_IS_DISPLAY_FOR_NONE)) {


For name of variable: RESOURCE_IS_DISPLAY_FOR_NONE is not bad. A suggestion for maybe a better one: DEFAULT_NO_VALUE

Problem with that is this class covers all parsing so it would need to cover default no value or none for all cases. Without checking all of the XML parameters, I'm not sure that is the case.

Ah, we may be looking at this from a different perspective. I'm not associating personally the 'NONE' value specifically to this one attribute. From what I can tell NONE is a magic valued supplied by the XML parser as we have the attribute defined as 'IMPLIED'. For me the concern is then knowing where 'NONE' came from. I'm pretty sure on a naive pass at this code, not looking at it in the context of the this PR, I would have probably assumed 'NONE' was going to be an explicitly defined value in XML. So really the recommendation is to make the magic 'NONE' value more explicit in one way or another, so we know that it is the XML parser generating for us when the attribute is not specified (as opposed to that being just a magic value that we coordinate between XML and gamecode). Hopefully that makes sense.

'NONE' is an explicitly defined parameter in the XML nothing to do with the XML parser or 'IMPLIED'. If the attribute isn't specified then you get an empty string. 'NONE' is used by map makers to specify the resource shouldn't be displayed for any players since the default (not specified, empty string) is that its displayed for all players.

Ah, so it is a magic value then. Is there any difference between NONE and not specifying the attribute?

Yeah, that's what I'm getting at. Not specifying (default) is you get an empty string which maps to showing the resource to all players. Specifying 'NONE' means the resource isn't displayed to any players. Reasoning being we want the default for all existing maps to be all players not none.

Ok, makes sense now, thank you

DanVanAtta · 2018-02-18T19:31:16Z

@ron-murhammer should be good to merge once conflicts are resolved.

ron-murhammer · 2018-02-19T04:25:39Z

Closing for #3034. Let's please try to minimize merging changes the end up with massive conflicts.

DanVanAtta · 2018-02-19T15:14:21Z

@ron-murhammer #3016 was opened to deconflict this PR, you only had to merge that : |

Sorry about the conflicts, this was the only open PR after source files were moved and #3016 was opened to help out. It really was two ways, deconflicting the source code move was also painful and I had to do that multiple times (and not without risk of doing it wrong on the Nth time).

Add new resource attribute isDisplayFor

c56b573

Remove preferred size parameter

8781ebf

DanVanAtta requested changes Feb 18, 2018

View reviewed changes

ssoloff reviewed Feb 18, 2018

View reviewed changes

DanVanAtta mentioned this pull request Feb 18, 2018

De-conflict: Add new resource attribute isDisplayFor #3016

Closed

Small clean ups

5d51f64

ssoloff approved these changes Feb 18, 2018

View reviewed changes

Add final and refactor method

d50edba

ron-murhammer assigned DanVanAtta Feb 18, 2018

DanVanAtta approved these changes Feb 18, 2018

View reviewed changes

DanVanAtta assigned ron-murhammer and unassigned DanVanAtta Feb 18, 2018

ron-murhammer closed this Feb 19, 2018

RoiEXLab deleted the Add_Resource_isDisplayFor_Property branch February 19, 2018 13:21

ssoloff unassigned ron-murhammer Feb 27, 2018

ssoloff removed the PR: <TESTING> label Feb 27, 2018

		@@ -14,4 +19,14 @@
		public Resource(final String name, final GameData data) {
		super(name, data);

Add new resource attribute isDisplayFor #3015

Add new resource attribute isDisplayFor #3015

Conversation

ron-murhammer commented Feb 17, 2018 • edited Loading

codecov-io commented Feb 17, 2018 • edited Loading

Codecov Report

DanVanAtta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssoloff Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ron-murhammer Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta commented Feb 18, 2018

ron-murhammer commented Feb 18, 2018 • edited Loading

ssoloff Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ron-murhammer commented Feb 18, 2018

DanVanAtta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ron-murhammer Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta commented Feb 18, 2018

ron-murhammer commented Feb 19, 2018

DanVanAtta commented Feb 19, 2018

ron-murhammer commented Feb 17, 2018 •

edited

Loading

codecov-io commented Feb 17, 2018 •

edited

Loading

DanVanAtta left a comment •

edited

Loading

ssoloff Feb 18, 2018 •

edited

Loading

ron-murhammer Feb 18, 2018 •

edited

Loading

ron-murhammer commented Feb 18, 2018 •

edited

Loading

ssoloff Feb 18, 2018 •

edited

Loading

ron-murhammer Feb 18, 2018 •

edited

Loading