Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new resource attribute isDisplayFor #3015

Closed
wants to merge 4 commits into from
Closed

Add new resource attribute isDisplayFor #3015

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c56b573
Add new resource attribute isDisplayFor
ron-murhammer Feb 17, 2018
8781ebf
Remove preferred size parameter
ron-murhammer Feb 17, 2018
5d51f64
Small clean ups
ron-murhammer Feb 18, 2018
d50edba
Add final and refactor method
ron-murhammer Feb 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/main/java/games/strategy/engine/data/GameParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@
*/
public final class GameParser {
private static final Class<?>[] SETTER_ARGS = {String.class};
private static final String RESOURCE_IS_DISPLAY_FOR_NONE = "NONE";

private final GameData data = new GameData();
private final Collection<SAXParseException> errorsSax = new ArrayList<>();
public static final String DTD_FILE_NAME = "game.dtd";
Expand Down Expand Up @@ -732,11 +734,25 @@ private void parseConnections(final List<Element> connections) throws GameParseE
}
}

private void parseResources(final Element root) {
getChildren("resource", root).stream()
.map(e -> e.getAttribute("name"))
.map(name -> new Resource(name, data))
.forEach(data.getResourceList()::addResource);
private void parseResources(final Element root) throws GameParseException {
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ron-murhammer ron-murhammer Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (isDisplayedFor.equalsIgnoreCase(RESOURCE_IS_DISPLAY_FOR_NONE)) {
players.clear();
} else if (!isDisplayedFor.isEmpty()) {
players.clear();
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);
}
players.add(player);
}
}
data.getResourceList().addResource(new Resource(name, data, players));
}
}

private void parseRelationshipTypes(final Element root) {
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/games/strategy/engine/data/Resource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package games.strategy.engine.data;

import java.util.ArrayList;
import java.util.List;

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

private List<PlayerID> players = new ArrayList<>();
Copy link
Member

@ssoloff ssoloff Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Updated.


/**
* Creates new Resource.
*
Expand All @@ -12,6 +17,17 @@ public class Resource extends NamedAttachable {
* game data
*/
public Resource(final String name, final GameData data) {
this(name, data, new ArrayList<>());
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public boolean isDisplayedFor(final PlayerID player) {
// TODO: remove null check on incompatible release
return players == null || players.contains(player);
}

}
22 changes: 12 additions & 10 deletions src/main/java/games/strategy/triplea/ui/ResourceBar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
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;

/**
Expand All @@ -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> resources = new ArrayList<>();
private final List<ResourceStat> resourceStats = new ArrayList<>();
private final List<JLabel> labels = new ArrayList<>();

public ResourceBar(final GameData data, final UiContext uiContext) {
Expand Down Expand Up @@ -51,7 +50,7 @@ private void setResources() {
if (resource.getName().equals(Constants.VPS)) {
continue;
}
resources.add(new ResourceStat(resource));
resourceStats.add(new ResourceStat(resource));
final JLabel label = new JLabel();
label.setBorder(BorderFactory.createEmptyBorder(0, 0, 0, 20));
labels.add(label);
Expand All @@ -64,15 +63,18 @@ public void gameDataChanged(final Change change) {
try {
final PlayerID player = gameData.getSequence().getStep().getPlayerId();
if (player != null) {
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 ResourceStat resourceStat = resourceStats.get(i);
final Resource resource = resourceStat.resource;
final JLabel label = labels.get(i);
final String quantity = resourceStat.getFormatter().format(resourceStat.getValue(player, gameData));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

label.setVisible(resource.isDisplayedFor(player));
try {
labels.get(i).setIcon(uiContext.getResourceImageFactory()
.getIcon(gameData.getResourceList().getResource(resources.get(i).getName()), true));
labels.get(i).setText(quantity);
labels.get(i).setToolTipText(resources.get(i).getName());
label.setIcon(uiContext.getResourceImageFactory().getIcon(resource, true));
label.setText(quantity);
label.setToolTipText(resourceStat.getName());
} catch (final IllegalStateException e) {
labels.get(i).setText(resources.get(i).getName() + " " + quantity);
label.setText(resourceStat.getName() + " " + quantity);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/games/strategy/engine/xml/game.dtd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
<!ELEMENT resource EMPTY>
<!ATTLIST resource
name ID #REQUIRED
isDisplayedFor CDATA #IMPLIED
>

<!ELEMENT unitList (unit+) >
Expand Down