15
\$\begingroup\$

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.
  • CardSet: Interface for adding a collection of CardModels to the game.
  • Event* and IEvent*: Part of the Event system in my Commons project
  • *Event: Various card events. Most of these classes are immutable.
  • ZomisList: Mostly used for getting a random element. Also contains a shuffle method (because GWT does not provide the Collections.shuffle method).

Usage / Test

You can find implementations of games on github. See for example CWars2CardsTest and SimpleGameTest.

Questions

My primary concerns are:

  • Generics. Am I over-using it or under-using it? The idea behind my usage is that all "outer" classes uses generics for their "inner" things. A Zone is bigger than a card, so it uses generics to know which kind of cards it can use. In some of the game implementations, a card uses covariant return types for getGame.

  • Card Creation. Currently, the code for creating a Card<M> is inside the CardModel (and it's subclasses). I have not yet been able to change this to a factory or similar. Any ideas of how to do that in a cleaner way?

  • Is the overall design good here?

Besides this, more or less any comments are welcome.

Code

I apologize for the wall of code, but I hope you will find this project interesting enough to review it. With great flexibility comes a whole lot of code lines.

This code can also be found on GitHub, where you will also find tests, dependencies, several actual game implementations, and more.

Card.java: (121 lines, 3556 bytes)

public class Card<M extends CardModel> {
    private final M model;

    protected CardZone<?> currentZone;

    protected Card(M model) {
        this.model = model;
    }

    public StackAction clickAction() {
        CardGame<?, ?> game = this.getGame();
        return game.getActionFor(this);
    }

    public M getModel() {
        return model;
    }

    @Override
    public String toString() {
        return "Card:" + model.toString();
    }

    public void moveAndReplaceWith(CardZoneLocation location, Card<M> card) {
        CardZone<?> destination = location.getZone();
        CardZone<?> zone = this.getCurrentZone();
        CardGame<?, ?> game = zone.getGame();
        if (game == null) {
            game = destination.getGame();
        }
        if (game == null) {
            throw new NullPointerException("Neither zone is connected to a game: " + zone + " --> " + destination);
        }

        ZoneChangeEvent event = new ZoneChangeEvent(this.currentZone, destination, this);
        game.executeEvent(event);
        game.executeEvent(new ZoneChangeEvent(card.getCurrentZone(), this.currentZone, card));

        @SuppressWarnings("unchecked")
        CardZone<Card<M>> dest = (CardZone<Card<M>>) event.getToCardZone();

        int myIndex = getCurrentZone().cardList().indexOf(this);
        int newCardOldIndex = card.getCurrentZone().cardList().indexOf(card);

        @SuppressWarnings("unchecked")
        List<Card<M>> list = (List<Card<M>>) this.getCurrentZone().cardList();
        list.set(myIndex, card);
        card.getCurrentZone().cardList().remove(newCardOldIndex);

        if (dest != null) {
            if (location.isTop()) {
                dest.cardList().addFirst(this);
            }
            else if (location.isBottom()) {
                dest.cardList().addLast(this);
            }
            else dest.cardList().add(location.getIndex(), this);
        }
        card.currentZone = this.currentZone;
        this.currentZone = dest;
    }

    public void zoneMoveOnBottom(CardZone<?> destination) {
        this.zoneMoveInternal(destination, false);
    }

    public void zoneMoveOnTop(CardZone<?> destination) {
        this.zoneMoveInternal(destination, true);
    }

    private void zoneMoveInternal(CardZone<?> destination, boolean top) {
        ZoneChangeEvent event = new ZoneChangeEvent(this.currentZone, destination, this);
        CardZone<?> zone = this.getCurrentZone();
        CardGame<?, ?> game = zone.getGame();
        if (game == null)
            game = destination.getGame();
        if (game == null)
            throw new NullPointerException("Neither zone is connected to a game: " + zone + " --> " + destination);

        game.executeEvent(event);
        event.getFromCardZone().cardList().remove(this);
        @SuppressWarnings("unchecked")
        CardZone<Card<M>> dest = (CardZone<Card<M>>) event.getToCardZone();
        if (dest != null) {
            if (top) dest.cardList().addFirst(this);
            else dest.cardList().addLast(this);
        }
        this.currentZone = dest;
    }

    public CardZone<?> getCurrentZone() {
        return currentZone;
    }

    public CardGame<?, ?> getGame() {
        if (currentZone == null)
            throw new NullPointerException("Card is not within a zone: " + this + ". Is it possible that this card has been moved to /dev/null?");
        return currentZone.getGame();
    }

    public String getDescription() {
        return getModel().getName();
    }

    public boolean isKnown() {
        return getCurrentZone().isKnown(getGame().getCurrentPlayer());
    }

    public Player getOwner() {
        if (currentZone == null)
            throw new NullPointerException("Card is not within a zone: " + this);
        return currentZone.getOwner();
    }

}

CardGame.java: (368 lines, 10999 bytes)

public class CardGame<P extends Player, M extends CardModel> implements EventListener {

    private ActionHandler actionHandler;

    private final Map<M, ActionProvider> actions;
    private final CardZone<Card<M>> actionZone;

    private final Map<String, M> availableCards = new HashMap<String, M>();

    private boolean callingOnEnd;

    private GamePhase currentPhase;
    private final IEventExecutor events;
    private boolean gameOver = false;
    private final List<GamePhase> phases = new ArrayList<GamePhase>();
    private final List<P> players = new LinkedList<P>();
    private Random random = new Random();
    private CardReplay replay;

    /**
     * The stack provides a way for actions to be processed one at a time.
     * Cannot be declared as a Deque interface because of GWT.
     */
    private final LinkedList<StackAction> stack = new LinkedList<StackAction>();

    private boolean started;

    private final List<CardZone<?>> zones = new ArrayList<CardZone<?>>();

    public CardGame() {
        this.events = new EventExecutorGWT();
        this.actionZone = new CardZone<Card<M>>("Actions");
        this.actionZone.setGloballyKnown(true);
        this.addZone(actionZone);
        this.actions = new HashMap<M, ActionProvider>();
    }

    public Card<M> addAction(M actionModel, ActionProvider action) {
        addCard(actionModel);
        actions.put(actionModel, action);
        return this.actionZone.createCardOnBottom(actionModel);
    }

    /**
     * A combination of {@link #addStackAction(StackAction)} and {@link #processStackAction()}
     * @param action {@link StackAction} to add and process.
     */
    public void addAndProcessStackAction(StackAction action) {
        this.addStackAction(action);
        this.processStackAction();
    }

    public void addCard(M cardModel) {
        if (cardModel == null)
            throw new IllegalArgumentException("Card cannot be null");
        if (this.availableCards.containsKey(cardModel.getName()))
            throw new IllegalStateException("A card with the name " + cardModel.getName() + " has already been added");
        this.availableCards.put(cardModel.getName(), cardModel);
    }

    @SuppressWarnings("unchecked")
    public <T extends CardGame<P, M>> void addCards(CardSet<T> cardSet) {
        cardSet.addCards((T) this);
    }

    protected void addPhase(GamePhase phase) {
        this.phases.add(phase);
    }

    protected void addPlayer(P player) {
        this.players.add(player);
        player.game = this;
    }

    /**
     * Add an action to the stack to be performed later. This does not save anything in history.
     * @param action Action to add to stack
     */
    public void addStackAction(StackAction action) {
        this.stack.addFirst(action);
    }

    protected CardZone<?> addZone(CardZone<?> zone) {
        if (zone == null)
            throw new IllegalArgumentException("Zone cannot be null");
        this.zones.add(zone);
        zone.game = this;
        return zone;
    }

    public boolean click(Card<?> card) {
        return clickPerform(card).actionIsPerformed();
    }

    public StackAction clickPerform(Card<?> card) {
        if (card == null)
            throw new NullPointerException("Card cannot be null");

        CardGame<?, ?> cardGame = card.getGame();
        StackAction action = card.clickAction();
        if (action.actionIsAllowed()) {
            replay.addMove(card);
        }
        addAndProcessStackAction(action);
        executeEvent(new CardPlayedEvent(card, cardGame, action));
        return action;
    }

    protected final void endGame() {
        if (this.isGameOver()) {
            return;
        }
        if (!this.executeEvent(new GameOverEvent(this)).isCancelled()) {
            this.gameOver = true;
        }
    }

    protected void executeEvent(IEvent event, int i) {
        getEvents().executeEvent(event, i);
    }
    protected <T extends IEvent> T executeEvent(T event) {
        return getEvents().executeEvent(event);
    }

    protected <T extends IEvent> T executeEvent(T event, Runnable runInBetween) {
        executeEvent(event, EventExecutorGWT.PRE);
        runInBetween.run();
        executeEvent(event, EventExecutorGWT.POST);
        return event;
    }

    public StackAction getActionFor(Card<?> card) {
        if (actionHandler == null)
            throw new IllegalStateException("No actionHandler has been set.");
        if (actions.containsKey(card.getModel())) {
            return actions.get(card.getModel()).get();
        }
        return actionHandler.click(card);
    }

    public CardZone<?> getActionZone() {
        return this.actionZone;
    }

    public GamePhase getActivePhase() {
        return this.currentPhase;
    }
    public M getCardModel(String name) {
        return getCards().get(name);
    }

    public Map<String, M> getCards() {
        return new HashMap<String, M>(availableCards);
    }

    @SuppressWarnings("unchecked")
    public P getCurrentPlayer() {
        GamePhase phase = getActivePhase();
        return (P) phase.getPlayer();
    }

    protected IEventExecutor getEvents() {
        return events;
    }

    public Player getFirstPlayer() {
        if (players.isEmpty())
            return null;
        return players.get(0);
    }

    protected List<GamePhase> getPhases() {
        return phases;
    }

    public List<P> getPlayers() {
        return Collections.unmodifiableList(players);
    }

    public List<CardZone<?>> getPublicZones() {
        return new ArrayList<CardZone<?>>(zones);
    }

    public final Random getRandom() {
        return this.random;
    }

    public CardReplay getReplay() {
        return replay;
    }

    protected List<StackAction> getStack() {
        return stack;
    }

    public List<Card<?>> getUseableCards(Player player) {
        if (actionHandler == null)
            throw new IllegalStateException("No actionHandler has been set.");
        List<Card<?>> list = this.actionHandler.getUseableCards(this, player);
        list.addAll(this.actionZone.cardList());
        return list;
    }

    public final boolean isGameOver() {
        return gameOver;
    }

    public boolean isNextPhaseAllowed() {
        return stack.isEmpty();
    }

    public boolean isStarted() {
        return started;
    }

    public boolean nextPhase() {
        if (!this.started)
            throw new IllegalStateException("Game is not started.");
        if (!this.isNextPhaseAllowed())
            return false;

        GamePhase previousPhase = getActivePhase();
        if (previousPhase == null) {
            this.setActivePhase(this.phases.get(0));
        }
        else {
            callingOnEnd = true;
            previousPhase.onEnd(this);
            callingOnEnd = false;
        }

        if (previousPhase == this.getActivePhase()) {
            // setActivePhase was not called from onEnd of the previous phase, so we need to find the next phase here
            int activePhase = this.phases.indexOf(getActivePhase());
            if (activePhase < 0) {
                throw new IllegalStateException("Phase does not appear in list of phases and did not change phase from GamePhase.onEnd: " + previousPhase);
            }
            int nextPhase = (activePhase + 1) % this.phases.size();
            GamePhase next = this.phases.get(nextPhase);
            setActivePhaseDirectly(next); // onEnd already called above so it should not be called again
        }
        return true;
    }

    protected void onStart() {}

    /**
     * Take the top {@link StackAction} of the stack and process it if it is allowed.
     * @return The {@link StackAction} that was removed from the stack and possibly processed.
     */
    public StackAction processStackAction() {
        if (this.isGameOver())
            return new InvalidStackAction("Game has already ended.");
        if (!this.started)
            throw new IllegalStateException("Game is not started. Did you forget to call startGame() ?");

        StackAction action = stack.isEmpty() ? null : stack.removeFirst();
        if (action == null) 
            action = new StackAction();

        if (action.actionIsAllowed()) {
            action.internalPerform();
            executeEvent(new AfterActionEvent(this, action));
        }
        else {
            action.onFailedPerform();
        }
        return action;
    }

    public void registerHandler(Class<? extends IEvent> eventType, EventHandlerGWT<? extends IEvent> handler) {
        getEvents().registerHandler(eventType, handler);
    }

    public <T extends IEvent> IEventHandler registerHandler(Class<? extends T> eventType, EventConsumer<T> handler) {
        return getEvents().registerHandler(eventType, handler);
    }

    public <T extends IEvent> IEventHandler registerHandler(Class<? extends T> eventType, EventConsumer<T> handler, int priority) {
        return getEvents().registerHandler(eventType, handler, priority);
    }

    public void removeHandler(IEventHandler listener) {
        getEvents().removeHandler(listener);
    }

    protected void setActionHandler(ActionHandler aiHandler) {
        if (aiHandler == null)
            throw new IllegalArgumentException("ActionHandler cannot be null.");
        this.actionHandler = aiHandler;
    }

    protected void setActivePhase(GamePhase phase) {
        if (!callingOnEnd) {
            callingOnEnd = true;
            // TODO: The `callingOnEnd` variable feels like a little dirty way to let phases go to the next phase by themselves, but it works.
            GamePhase active = getActivePhase();
            if (active != null)
                active.onEnd(this);
        }
        callingOnEnd = false;
        setActivePhaseDirectly(phase);
    }

    /**
     * Go to the next phase without calling onEnd on the current phase. Will call onStart on the new phase.
     * @param phase The phase to go to.
     */
    protected void setActivePhaseDirectly(GamePhase phase) {
        GamePhase oldPhase = getActivePhase();
        this.executeEvent(new PhaseChangeEvent(this, oldPhase, phase), EventExecutorGWT.PRE);
        this.currentPhase = phase;
        GamePhase newPhase = getActivePhase();
        newPhase.onStart(this);
        this.executeEvent(new PhaseChangeEvent(this, oldPhase, newPhase), EventExecutorGWT.POST);
    }

    public final void setRandom(Random random) {
        this.random = random;
    }

    public final void setRandomSeed(long seed) {
        this.random = new Random(seed);
    }

    public int stackSize() {
        return this.stack.size();
    }

    public final void startGame() {
        if (this.started)
            throw new IllegalStateException("Game is already started.");
        if (this.phases.isEmpty())
            throw new IllegalStateException("Game does not have any phases");
        this.replay = new CardReplay(this);
        this.started = true;
        this.onStart();
        if (this.getActivePhase() == null) {
            this.setActivePhase(this.phases.get(0));
        }
    }
}

CardModel.java: (37 lines, 866 bytes)

public class CardModel implements Comparable<CardModel> {

    private final String name;

    public CardModel(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }

    protected <E extends CardModel> Card<E> createCardInternal(CardZone<?> initialZone) {
        @SuppressWarnings("unchecked")
        Card<E> card = new Card<E>((E) this); // TODO: Possibly create the cards themselves elsewhere...
        card.currentZone = initialZone;
        return card;
    }

    @Override
    public int compareTo(CardModel o) {
        if (name == null || o.name == null)
            throw new NullPointerException("Name cannot be null");
        return name.compareTo(o.getName());
    }
    @Override
    public String toString() {
        return this.getName();
    }

    @Override
    public int hashCode() {
        return this.getName().hashCode();
    }
}

CardZone.java: (201 lines, 5233 bytes)

public class CardZone<E extends Card<?>> implements Comparable<CardZone<E>>, Iterable<E> {

    private int timesCopied;

    CardGame<?, ?> game;

    private final Map<Player, Boolean> known = new HashMap<Player, Boolean>();
    private final LinkedList<E> cards = new LinkedList<E>();
    private final String name;
    private final Player owner;

    private boolean knownGlobal;

    public CardZone(String zoneName, Player owner) {
        this.name = zoneName;
        this.owner = owner;
    }
    public CardZone(String zoneName) {
        this(zoneName, null);
    }
    public void setGloballyKnown(boolean knowledge) {
        this.knownGlobal = knowledge;
    }
    public CardZone<E> setKnown(Player player, boolean knowledge) {
        this.known.put(player, knowledge);
        return this;
    }

    public boolean isKnown(Player player) {
        Boolean b = known.get(player);
        return (b == null ? knownGlobal : b);
    }

    public boolean contains(E card) {
        return cards.contains(card);
    }

    public LinkedList<E> cardList() {
        return cards;
    }

    @Override
    public final String toString() {
        return "Zone{" + getName() + "}";
    }

    public String getFullName() {
        return owner == null ? getName() : "Player" + getOwner().getIndex() + " " + getName();
    }

    public String getName() {
        return name;
    }

    @Override
    public int compareTo(CardZone<E> arg0) {
        return name.compareTo(arg0.name);
    }

    public CardGame<?, ?> getGame() {
        return game;
    }
    public void shuffle() {
        if (game == null)
            throw new NullPointerException("CardZone doesn't have a game");
        this.shuffle(game.getRandom());
    }
    public void shuffle(Random random) {
        ZomisList.shuffle(cards, random);
        this.executeEvent(new ZoneShuffleEvent(this));
    }

    public void sort(Comparator<E> comparator) {
        Collections.sort(this.cards, comparator);
        this.executeEvent(new ZoneSortEvent(this));
    }

    public E createCardOnTop(CardModel cardModel) {
        if (cardModel == null)
            throw new NullPointerException("CardModel cannot be null");

        @SuppressWarnings("unchecked")
        E card = (E) cardModel.createCardInternal(this); // TODO: Possibly use a "CardFactory" somewhere...
        this.cards.addFirst((E) card);
        this.executeEvent(new CardCreatedEvent(card));
        return card;
    }
    private void executeEvent(IEvent event) {
        if (this.game != null)
            this.game.executeEvent(event);
    }
    public E createCardOnBottom(CardModel cardModel) {
        if (cardModel == null)
            throw new NullPointerException("CardModel cannot be null");
        @SuppressWarnings("unchecked")
        E card = (E) cardModel.createCardInternal(this);
        this.cards.addLast(card);
        this.executeEvent(new CardCreatedEvent(card));
        return card;
    }
    public E getTopCard() {
        if (cards.isEmpty())
            return null;
        return this.cards.getFirst();
    }

    public E getBottomCard() {
        if (cards.isEmpty())
            return null;
        return this.cards.getLast();
    }

    public CardZone<E> extractTopCards(int number) {
        CardZone<E> copy = this.createEmptyCopy();
        for (int i = 0; i < number; i++) {
            this.getTopCard().zoneMoveOnBottom(copy);
        }
        return copy;
    }
    public CardZone<E> extractBottomCards(int number) {
        CardZone<E> copy = this.createEmptyCopy();
        for (int i = 0; i < number; i++) {
            this.getBottomCard().zoneMoveOnTop(copy);
        }
        return copy;
    }
    /**
     * Reverse the ordering of the cards in this zone
     * @return Returns self
     */
    public CardZone<E> reverse() {
        Collections.reverse(cards);
        this.executeEvent(new ZoneReverseEvent(this));
        return this;
    }
    public void moveToTopOf(CardZone<E> destination) {
        List<E> list = new ArrayList<E>(cards);
        Collections.reverse(list);
        for (Card<?> card : list) {
            card.zoneMoveOnTop(destination);
        }
    }
    public void moveToBottomOf(CardZone<E> destination) {
        for (E card : new ArrayList<E>(cards)) {
            card.zoneMoveOnBottom(destination);
        }
    }

    private CardZone<E> createEmptyCopy() {
        CardZone<E> zone = new CardZone<E>(this.getName() + "-Copy" + ++timesCopied, owner);
        zone.setGloballyKnown(this.knownGlobal);
        for (Entry<Player, Boolean> ee : this.known.entrySet()) {
            zone.setKnown(ee.getKey(), ee.getValue());
        }
        return zone;
    }
    public boolean isEmpty() {
        return this.cards.isEmpty();
    }
    public int size() {
        return this.cards.size();
    }

    public Player getOwner() {
        return owner;
    }

    @Override
    public Iterator<E> iterator() {
        return cards.iterator();
    }

    public boolean containsModel(CardModel c) {
        for (E card : this) {
            if (c.equals(card.getModel())) {
                return true;
            }
        }
        return false;
    }
}

CardZoneLocation.java: (48 lines, 1019 bytes)

public class CardZoneLocation {

    private final CardZone<?> zone;
    private final int index;

    private static final int BOTTOM = Integer.MAX_VALUE;
    private static final int TOP = -1;

    public CardZoneLocation(CardZone<?> zone, int index) {
        this.zone = zone;
        this.index = index;
    }

    public int getIndex() {
        return index;
    }

    public CardZone<?> getZone() {
        return zone;
    }

    public boolean isTop() {
        return this.index == TOP;
    }

    public boolean isBottom() {
        return this.index == BOTTOM;
    }

    public static CardZoneLocation nowhere() {
        return new CardZoneLocation(null, BOTTOM);
    }

    public static CardZoneLocation bottomOf(CardZone<?> zone) {
        return new CardZoneLocation(zone, BOTTOM);
    }

    public static CardZoneLocation topOf(CardZone<?> zone) {
        return new CardZoneLocation(zone, TOP);
    }

    public static CardZoneLocation indexIn(CardZone<?> zone, int index) {
        return new CardZoneLocation(zone, index);
    }

}

GamePhase.java: (35 lines, 517 bytes)

public class GamePhase {

    private final Player player;

    @Deprecated
    public GamePhase() {
        this(null);
    }

    public GamePhase(Player pl) {
        this.player = pl;
    }

    public Player getPlayer() {
        return player;
    }

    @Override
    public String toString() {
        if (player == null)
            return "Phase-null";
        return "Phase-" + player.getName();
    }

    public void onStart(CardGame<?, ?> game) {

    }
    public void onEnd(CardGame<?, ?> game) {

    }

}

Player.java: (64 lines, 1542 bytes)

public class Player implements HasResources {

    CardGame<? extends Player, ?> game;

    private String name;
    private final ResourceMap resources = new ResourceMap(true);

    public CardGame<?, ?> getGame() {
        if (game == null)
            throw new IllegalStateException("Player was not added to game correctly.");
        return game;
    }

    @Override
    public ResourceMap getResources() {
        return resources;
    }
    public <E extends Player> List<E> getOpponents() {
        @SuppressWarnings("unchecked")
        List<E> players = (List<E>) game.getPlayers();
        int index = players.indexOf(this);
        List<E> before = players.subList(0, index);
        List<E> after = players.subList(index + 1, players.size());

        List<E> result = new ArrayList<E>(after.size() + before.size());
        result.addAll(after);
        result.addAll(before);
        return result;
    }

    public Player getNextPlayer() {
        return getOpponents().get(0);
    }

    public String getName() {
        return name;
    }
    public Player setName(String name) {
        this.name = name;
        return this;
    }

    @Override
    public String toString() {
        return "Player-" + this.getName();
    }

    public int getIndex() {
    return getGame().getPlayers().indexOf(this);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I only looked at it briefly, but it seems very complex for a library for card games. I don't think I would use it if I were to write a card game. I would rather write everything from scratch, specifically for my game. I don't think the problem is with your implementation. I only know "classical" card games, but maybe the other kinds of games are too different and the common abstraction ends up being too... abstract. \$\endgroup\$ – toto2 Jun 10 '14 at 22:11
  • \$\begingroup\$ I see many advantages in making an abstraction for the common parts though. The view part of the code uses this abstraction a lot. I believe some other parts, such as calculating probabilities, can also benefit from the abstraction. I will later on post a follow-up to this question showing an example of how it can be easier to make a card game (although there's a possibility you might consider that "too abstract" as well). \$\endgroup\$ – Simon Forsberg Jun 10 '14 at 22:34
  • \$\begingroup\$ I'd be interesting in seeing an application; maybe some subclasses for "classical" card games. As I said, I'm not familiar with the "new" card games, so this library seems overly complicated to me. \$\endgroup\$ – toto2 Jun 11 '14 at 0:21
  • 1
    \$\begingroup\$ throwing an exception in compareTo? ouch. Is name == null any different than name == "" as a practical matter? Off hand I'd say they're all equal for the purposes of comparing cards. You're performing data validation, and incompletely at that, in a comparable implementation! \$\endgroup\$ – radarbob Jun 20 '14 at 0:19
  • \$\begingroup\$ @radarbob You're completely right! If anywhere, I should throw the exception in the constructor where it's initialized in the first place. \$\endgroup\$ – Simon Forsberg Jun 23 '14 at 16:00
8
\$\begingroup\$

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<ClassicCardModel>, and MagicTheGatheringCardModel.createCardInternal() should return Card<MagicTheGatheringCardModel>. Generics are the way to achieve this, but there's a surprise: getThis

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

    protected Card<M> createCardInternal(CardZone<?> initialZone) {
        Card<M> card = new Card<M>(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<M> {
    // This cardZone accepts any cards of CardModel M, or
    // cards of CardModels that extend M.
    public void add(Card<? extends M> card);
}

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

    protected void createCardInternal(CardZone<M> initialZone) {
        Card<M> card = new Card<M>(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<M extends CardModel> {
    // This method is provided by the LEAF classes derived from CardModel
    abstract M getThis();

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

This may or may not be a useful idea for you -- you can put Card<ExtendedType> into CardZone<BaseType>, 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.

\$\endgroup\$
  • \$\begingroup\$ MagicTheGatheringCardModel.createCardInternal() should return MagicCard, which extends Card<MagicCardModel>. I hope that doesn't modify your suggestions? Honestly, I'm not even sure myself why I'm overriding hashCode and implementing Comparable in CardModel... I think I'll remove that and add it again if something breaks :) \$\endgroup\$ – Simon Forsberg Jun 23 '14 at 16:08
  • \$\begingroup\$ My answer was supposed to refer to Card<MagicTheGatheringCardModel>, but Markdown ate my homework. Factory<MagicCard> creates MagicCard is a perfectly reasonable idiom, but correctly integrating that with Factory<M> creates Card<M>... it may be the the right answer depends a lot on your context. My suggestion would be to take a swing at it, and create a new topic with what you have learned. \$\endgroup\$ – VoiceOfUnreason Jun 23 '14 at 18:23
  • \$\begingroup\$ For instance, it might be that the factory is really Factory<C extends Card<M>,M>, and SimpleFactory<M> extends Factory<Card<M>,M> and then MagicFactory extends Factory<MagicCard,MagicCardModel> and so on. \$\endgroup\$ – VoiceOfUnreason Jun 23 '14 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.