Zones and Cards

Description

This is the common code for all my Card Game projects. It is intended to be very flexible. This is related to the recent Community Challenge

The kinds of Card Games supported by this code is everything from Hearts to Castle Wars to some more Trading Card Games similar to Magic: The Gathering. This is because I tried to focus on what unites them all: It's all about various kinds of cards located in different zones.

I am using Java 8 to compile this, although because of Android and GWT support (through retrolambda plugin for Android and a fork of GWT), I only have restricted access to the Java 8 things to use (No streams! Only some of the Function interfaces). GWT also does not allow String.format :(

Class Summary (926 lines in 8 files, making a total of 24970 bytes)

• Card: The actual cards that exists in different zones and that move between zones

• CardModel: What the actual cards represent, which kind of card it is

• CardZone: Player Hands, battlefields, discard piles (both shared and player-specific), etc. They are all various zones.

• CardZoneLocation: A location in a CardZone. Top, bottom, or a specific index.

• GamePhase: For example setting-up phases (such as exchanging cards before game is ready to play), player-specific phases to specify who's turn it is, phase in which only some actions may performed.

• Player: Can have player-specific zones, often have resources

• CardGame: Contains the players, zones, card models, everything!

Summary of classes not shown here

• StackAction: When clicking/playing/using a card, a stack action is created to actually perform something. The subclasses of this class are responsible for what happens when a card is used.

• ActionHandler, ActionProvider: Interfaces for providing StackActions.

• InvalidStackAction: A subclass of StackAction that is never allowed.

• ResourceMap, HasResources: A resource system, instead of having a whole lot of getters and setters for gold, mana, health, etc.

• C

I like that you've factored out the idea of the CardModel being separate from the instances of the cards.

It's not at all clear to me why CardModel is trying to support Comparable. The ordering of cards is going to vary with the game that is being played, and even within the game, so I'm not convinced that any individual card model can really know if it is before or after another one.

Similarly, it's not clear that you should be over-riding hashCode; it really depends on whether there should be more than one "jack of spades" model running about. If all "jack of spades" are the same, then maybe the answer is that your specific cards are enums that share implementation details.

Which means that the public interface of CardModel is basically just Named. Which in turn makes me skeptical that it is really a public abstraction on its own. It looks as though its real purpose is to provide a common implementation of createCardInternal() to classes that extend it. In other words, it's an AbstractFactory

The unchecked cast warning is the compiler's way of telling you that it doesn't think you know what you are doing. The code is fine (after all, generics are syntactic sugar), but there are a few more hoops to jump through.

In particular, it looks as though what you want is that ClassicCardModel.createCardInternal() should return Card, and MagicTheGatheringCardModel.createCardInternal() should return Card. Generics are the way to achieve this, but there's a surprise: getThis

public abstract class CardModel {
    // This method is provided by the LEAF classes derived from CardModel
    abstract M getThis();

    protected Card createCardInternal(CardZone initialZone) {
        Card card = new Card(getThis());
        card.currentZone = initialZone;
        return card;
    }
}

Oblique, I admit, but a standard idiom. It's seen most often in builder patterns, which may be something you want to consider as an alternative to bind card models to the correct instance.

It also strikes me a bit odd that we assign the initialZone to the card, without also telling the zone that the card is there? Logically, I would expect a CardZone to contain cards, and indeed there is a List present. But the presence of CardZone.cardList() is a Very Bad Sign[tm]. I would expect to see something more like....

public interface CardZone {
    // This cardZone accepts any cards of CardModel M, or
    // cards of CardModels that extend M.
    public void add(Card card);
}

public abstract class CardModel {
    // This method is provided by the LEAF classes derived from CardModel
    abstract M getThis();

    protected void createCardInternal(CardZone initialZone) {
        Card card = new Card(getThis());
        initialZone.add(card);
    }
}

Now, CardZone is effectively a container, and it's reasonable to want to put Card into a container of Card. You can achieve that idea by using a lower bounded wildcard...

public abstract class CardModel {
    // This method is provided by the LEAF classes derived from CardModel
    abstract M getThis();

    // This signature accepts any CardZone into which Card can be added
    protected void createCardInternal(CardZone initialZone) {
        Card card = new Card(getThis());
        initialZone.add(card);
    }
}

This may or may not be a useful idea for you -- you can put Card into CardZone, but when it comes back out, it's going to look like a BaseType - consumers will have to downcast if they want to access the extended interface. It might instead be that all CardZones for a game necessarily share the same CardModel, in which case, this is all irrelevant. But if it is a useful idea, you definitely want to look at the builder pattern

I haven't reviewed the rest, but it is my strong suspicion that all of your other unbounded types should also be bounded.

It looks to me as though Card.moveAndReplaceWith() needs an overhaul - it is doing too many different things: it's creating and firing events, and manipulating lists that don't belong to it, and the suppressed warnings are a big code smell. Another hint that the abstraction is badly tangled is that "this" card is doing work for "that" card. Really, that logic looks a lot more like a dealer or player than it does a card.

StackExchange Code Review Q#53921, answer score: 8