Hearthstone Style Deck Stats for TCG

Right now I have this messy method to collect the deck statistics while building a window that will display those statistics. These statistics are things like the type and mana cost of the card. I know that this method is too large, so I would like to get some advice for how to make it smaller.

The challenge is that I have to check if the current property in the for loop matches certain predetermined strings before putting the updated information back into the map. This is messy now, but it will get much messier later when even more properties need to be evaluated.

Here is the method:

```
private void buildDeckInfoBox() {
this.deckInfoLabelBox.getChildren().clear();

Map manaCostValues = new HashMap<>();
Map scrapCostValues = new HashMap<>();
Map creatureTypes = new HashMap<>();

for (int cardId : this.activeDeckConfig.getChosen().keySet()) {

CardInfoMessage card = this.cardList.get(cardId);
int cardCount = this.activeDeckConfig.getChosen().get(cardId);

for (Map.Entry entry : card.getProperties().entrySet()) {
String key = entry.getKey();
String value = String.valueOf(entry.getValue());

//The value is used as the key for the new map
//increment the number of cards and put it back in the map
if (key.equals("creatureType")) {
if (creatureTypes.get(value) == null) {
creatureTypes.put(value, cardCount);
} else {
int creatureCount = creatureTypes.get(value);
int newCreatureCount = creatureCount + cardCount;
creatureTypes.put(value, newCreatureCount);
}
} else if (key.equals("MANA_COST")) {
if (manaCostValues.get(Integer.parseInt(value)) == null) {
manaCostValues.put(Integer.parseInt(value), cardCount);
} else {
int manaCostCount = manaCostValues.get(In

First of all, there are only three specific keys that you are interested in, so there is no need to loop over the map. Just grab the keys that are interesting and see if they are null or not.

Secondly, it's possible to extract a method for increasing the actual card counts in each map.

Object creatureType = card.getProperties().get("creatureType");
Object manaCost = card.getProperties().get("MANA_COST");
Object scrapCost = card.getProperties().get("SCRAP_COST");
if (creatureType != null) {
    increase(creatureTypes, (String) creatureType, cardCount);
}
if (manaCost != null) {
    increase(manaCostValues, Integer.parseInt(String.valueOf(manaCost)), cardCount);
}
if (scrapCost != null) {
    increase(scrapCostValues, Integer.parseInt(String.valueOf(scrapCost)), cardCount);
}

Where the increase method is:

private  void increase(Map map, T key, int cardCount) {
    if (map.get(key) == null) {
        map.put(key, cardCount);
    } else {
        int oldValue = map.get(key);
        int newValue = oldValue + cardCount;
        map.put(key, newValue);
    }
}

Now, the folks over at Oracle noticed that this was a very common pattern in code, so in Java 8 they introduced the Map.merge method:

private  void increase(Map map, T key, int cardCount) {
    map.merge(key, cardCount, (a, b) -> a + b);
}

But what about this pattern?

Object creatureType = card.getProperties().get("creatureType");
if (creatureType != null) {
    increase(creatureTypes, (String) creatureType, cardCount);
}

Well, the new Java 8 Optional class to the rescue!

Optional.ofNullable(card.getProperties().get("creatureType"))
    .ifPresent(obj -> increase(creatureTypes, (String) obj, cardCount));

So your entire code can be changed to:

private void buildDeckInfoBox() {
    this.deckInfoLabelBox.getChildren().clear();

    Map manaCostValues = new HashMap<>();
    Map scrapCostValues = new HashMap<>();
    Map creatureTypes = new HashMap<>();

    for (int cardId : this.activeDeckConfig.getChosen().keySet()) {

        CardInfoMessage card = this.cardList.get(cardId);
        int cardCount = this.activeDeckConfig.getChosen().get(cardId);

        Optional.ofNullable(card.getProperties().get("creatureType"))
            .ifPresent(obj -> increase(creatureTypes, (String) obj, cardCount));
        Optional.ofNullable(card.getProperties().get("MANA_COST"))
            .ifPresent(obj -> increase(manaCostValues, (Integer) obj, cardCount));
        Optional.ofNullable(card.getProperties().get("SCRAP_COST"))
            .ifPresent(obj -> increase(scrapCostValues, (Integer) obj, cardCount));
    }

    for (int manaCost : manaCostValues.keySet()) {
        Label manaCostLabel = new Label();
        manaCostLabel.setText(String.format("Mana Cost = %d, Count = %d", manaCost, manaCostValues.get(manaCost)));
        this.deckInfoLabelBox.getChildren().add(manaCostLabel);
    }
    for (int scrapCost : scrapCostValues.keySet()) {
        Label scrapCostLabel = new Label();
        scrapCostLabel.setText(String.format("Scrap Cost = %d, Count = %d", scrapCost, scrapCostValues.get(scrapCost)));
        this.deckInfoLabelBox.getChildren().add(scrapCostLabel);
    }
    for (String creatureType : creatureTypes.keySet()) {
        Label creatureTypeLabel = new Label();
        creatureTypeLabel.setText(String.format("Creature Type %s, Count = %d", creatureType, creatureTypes.get(creatureType)));
        this.deckInfoLabelBox.getChildren().add(creatureTypeLabel);
    }
}

private  void increase(Map map, T key, int cardCount) {
    map.merge(key, cardCount, (a, b) -> a + b);
}

It is possible to refactor those last three for loops as well, but I am not sure if it is entirely worth it. Perhaps I'll show how to do that some other time.

StackExchange Code Review Q#68923, answer score: 3