10
\$\begingroup\$

Building on the older building block, Trading Card Game's Hand class and tests, I decided it was time to implement a HandView, which can be implemented by a GUI for example. The view need only be notified if a structural change happens in the "parent" class.

I have taken the following facts into consideration:

  • Loose coupling: I don't want the HandView to be a member of the Hand class
  • Composition over inheritance
  • Decoration to provide loose coupling.
  • Hand is a collection of cards semanticaly, hence it implements Collection<Card>.

Needless to say, I am not entirely happy with this approach as it is quite bloated and hence I am looking at an event bus driven approach, one example of such is Guava Libraries EventBus.

I haven't bothered with the unit tests for these classes yet as they are still all very experimental.

HandView:

public interface HandView {
    public void onCardAdded(final Card card);

    public void onCardPlayed(final int cardIndex);

    public void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo);
}

Hand:

public interface Hand extends Collection<Card> {
    public boolean isFull();

    @Override
    public boolean add(final Card card);

    public Card get(final int index);

    public Card play(final int index);

    public void swap(final int indexOne, final int indexTwo);

    @Override
    public String toString();

    @Override
    public Iterator<Card> iterator();

    @Override
    public Spliterator<Card> spliterator();

    @Override
    public int size();

    @Override
    public void forEach(final Consumer<? super Card> action);

    public static Hand newWithCapacity(final int capacity) {
        return new HandImpl(capacity);
    }

    public static Hand decorateHand(final Hand hand, final HandView handView) {
        Objects.requireNonNull(hand);
        Objects.requireNonNull(handView);
        return new Hand() {
            @Override
            public boolean isFull() {
                return hand.isFull();
            }

            @Override
            public boolean add(final Card card) {
                boolean result = hand.add(card);
                if (result) {
                    handView.onCardAdded(card);
                }
                return result;
            }

            @Override
            public Card get(final int index) {
                return hand.get(index);
            }

            @Override
            public Card play(final int index) {
                Card result = hand.play(index);
                handView.onCardPlayed(index);
                return result;
            }

            @Override
            public void swap(final int indexOne, final int indexTwo) {
                hand.swap(indexOne, indexTwo);
                handView.onCardsSwapped(indexOne, indexTwo);
            }

            @Override
            public Iterator<Card> iterator() {
                return hand.iterator();
            }

            @Override
            public Spliterator<Card> spliterator() {
                return hand.spliterator();
            }

            @Override
            public int size() {
                return hand.size();
            }

            @Override
            public void forEach(final Consumer<? super Card> action) {
                hand.forEach(action);
            }

            @Override
            public boolean isEmpty() {
                return hand.isEmpty();
            }

            @Override
            public boolean contains(final Object object) {
                return hand.contains(object);
            }

            @Override
            public Object[] toArray() {
                return hand.toArray();
            }

            @Override
            public <T> T[] toArray(final T[] array) {
                return hand.toArray(array);
            }

            @Override
            public boolean remove(final Object object) {
                return hand.remove(object);
            }

            @Override
            public boolean containsAll(final Collection<?> collection) {
                return hand.containsAll(collection);
            }

            @Override
            public boolean addAll(final Collection<? extends Card> collection) {
                return hand.addAll(collection);
            }

            @Override
            public boolean removeAll(final Collection<?> collection) {
                return hand.removeAll(collection);
            }

            @Override
            public boolean retainAll(final Collection<?> collection) {
                return hand.retainAll(collection);
            }

            @Override
            public void clear() {
                hand.clear();
            }
        };
    }
}

class HandImpl extends AbstractCollection<Card> implements Hand, Collection<Card> {
    private final List<Card> list = new ArrayList<>();
    private final int capacity;

    protected HandImpl(final int capacity) {
        this.capacity = Arguments.requirePositive(capacity, "capacity");
    }

    @Override
    public boolean isFull() {
        return (list.size() == capacity);
    }

    @Override
    public boolean add(final Card card) {
        Objects.requireNonNull(card);
        States.requireFalse(isFull(), "hand is full");
        list.add(card);
        return true;
    }

    @Override
    public Card get(final int index) {
        checkIndex(index);
        return list.get(index);
    }

    @Override
    public Card play(final int index) {
        checkIndex(index);
        return list.remove(index);
    }

    @Override
    public void swap(final int indexOne, final int indexTwo) {
        checkIndex(indexOne);
        checkIndex(indexTwo);
        Collections.swap(list, indexOne, indexTwo);
    }

    @Override
    public String toString() {
        return Hand.class.getSimpleName() + "(" + capacity + ", " + list + ")";
    }

    @Override
    public Iterator<Card> iterator() {
        return list.iterator();
    }

    @Override
    public Spliterator<Card> spliterator() {
        return list.spliterator();
    }

    @Override
    public int size() {
        return list.size();
    }

    @Override
    public void forEach(final Consumer<? super Card> action) {
        Objects.requireNonNull(action);
        list.forEach(action);
    }

    private void checkIndex(final int index) {
        Arguments.requireIndexInRange(index, 0, size());
    }
}

It can be instantiated with:

Hand hand = Hand.newWithCapacity(5);

or

Hand hand = Hand.decorateHand(Hand.newWithCapacity(5), /* concrete hand view */);
\$\endgroup\$
2
  • 1
    \$\begingroup\$ "I haven't bothered with the unit tests for these classes yet as they are still all very experimental." If you do not write tests first; tests, instead of verifying some preexisting requirement from that component, just bake in the current implementation. Does Hand really need to extend Collection? Do I really need to implement spliterator() for a collection that won't have more than a dozen elements? Let the requirements decide. \$\endgroup\$ Apr 29 '14 at 11:45
  • \$\begingroup\$ @abuzittingillifirca The default implementations of iterator(), spliterator() and forEach() are not good enough, therefore it is very much advisory to override them. As the List already does this, it is just a minor detail to delegate them explicitely to that list. \$\endgroup\$
    – skiwi
    Apr 29 '14 at 11:50
4
\$\begingroup\$

I will provide some comments here regarding your code, and at the end I will provide "My version" (with some of my comments taken into consideration).


Composition over inheritance

Although I understand that you mean that you want to use composition over inheritance for your HandView, your HandImpl (and your Hand itself) is using inheritance:

class HandImpl extends AbstractCollection<Card>

There is a bad aspect of letting your Hand class implement Collection: Completely outside of your class, somewhere far away in your code, would you want code which resides in a galaxy far far away to be allowed to call someHand.clear(); ? You probably don't. Which is why implementing Collection can be a bad idea. By not implementing Collection, everything has to go through the methods that you provide, such as someHand.play(index).

You say that Hand is a collection of cards semanticaly, hence it implements Collection. and sure, I agree with that. Hand is a collection of cards, but should it be used as such by outside code? Not in my opinion.


Because of your capacity in HandImpl, and that you're using an underlying ArrayList, it would be good to initialize the list to a specific capacity:

this.list = new ArrayList<>(capacity);

Guava provides a ForwardingCollection class, using it would greatly reduce the number of lines in your code.


I think that you are over-using your own checkIndex and other validations. You can remove them in the below cases (and probably some others too) to let the default exception be thrown (which will be very similar to the own you throw manually). Removing these additional checks you have added also can remove the need for overriding the method entirely, which will reduce some clutter.

@Override
public Card get(final int index) {
    checkIndex(index);
    return list.get(index);
}

@Override
public void forEach(final Consumer<? super Card> action) {
    Objects.requireNonNull(action);
    list.forEach(action);
}

Overriding methods in an interface. These lines in your Hand interface can be removed entirely, they don't provide anything extra since Hand extends Collection<Card>.

@Override
public String toString();

@Override
public Iterator<Card> iterator();

@Override
public Spliterator<Card> spliterator();

@Override
public int size();

@Override
public void forEach(final Consumer<? super Card> action);

Loose coupling: I don't want the HandView to be a member of the Hand class

IMO, whether or not it's a member of the Hand class does not affect coupling. HandView is an interface, it is already loosely coupled.

This method however, in your Hand interface, provides some coupling:

public static Hand newWithCapacity(final int capacity) {
    return new HandImpl(capacity);
}

I don't think this method belongs in your interface. Perhaps in a HandFactory (which might be a bit overkill). Or in your Game class. Or somewhere else. This is just my opinion though.


My version:

This code is still quite similar to your code. The hand still implements Collection<Card>, but the number of lines has been reduced by about 25-30 % (I might also have changed a few things to get it to compile, as I don't have some of your other classes, so read and see what I have done, and apply the things you want to your own code).

interface HandView {
    public void onCardAdded(final Card card);

    public void onCardPlayed(final int cardIndex);

    public void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo);
}

public interface Hand extends Collection<Card> {
    public boolean isFull();

    public Card get(final int index);

    public Card play(final int index);

    public void swap(final int indexOne, final int indexTwo);

    public static Hand newWithCapacity(final int capacity) {
        return new HandImpl(capacity);
    }

    public static Hand decorateHand(final Hand hand, final HandView handView) {
        Objects.requireNonNull(hand);
        Objects.requireNonNull(handView);
        return new ForwardingHand(hand) {
            @Override
            public boolean add(final Card card) {
                boolean result = hand.add(card);
                if (result) {
                    handView.onCardAdded(card);
                }
                return result;
            }

            @Override
            public Card play(final int index) {
                Card result = hand.play(index);
                handView.onCardPlayed(index);
                return result;
            }

            @Override
            public void swap(final int indexOne, final int indexTwo) {
                hand.swap(indexOne, indexTwo);
                handView.onCardsSwapped(indexOne, indexTwo);
            }
        };
    }
}

class HandImpl extends ForwardingCollection<Card> implements Hand {
    private final List<Card> list = new ArrayList<>();
    private final int capacity;

    protected HandImpl(final int capacity) {
        this.capacity = capacity;
    }

    @Override
    public boolean isFull() {
        return (list.size() == capacity);
    }

    @Override
    public boolean add(final Card card) {
        Objects.requireNonNull(card);
        if (isFull())
            throw new IllegalArgumentException();
        list.add(card);
        return true;
    }

    @Override
    public Card get(final int index) {
        return list.get(index);
    }

    @Override
    public Card play(final int index) {
        return list.remove(index);
    }

    @Override
    public void swap(final int indexOne, final int indexTwo) {
        Collections.swap(list, indexOne, indexTwo);
    }

    @Override
    public String toString() {
        return Hand.class.getSimpleName() + "(" + capacity + ", " + list + ")";
    }

    @Override
    protected Collection<Card> delegate() {
        return list;
    }
}

class ForwardingHand extends ForwardingCollection<Card> implements Hand {

    private final Hand  delegate;

    public ForwardingHand(Hand hand) {
        this.delegate = hand;
    }

    @Override
    public boolean isFull() {
        return delegate.isFull();
    }

    @Override
    public Card get(int index) {
        return delegate.get(index);
    }

    @Override
    public Card play(int index) {
        return delegate.play(index);
    }

    @Override
    public void swap(int indexOne, int indexTwo) {
        delegate.swap(indexOne, indexTwo);
    }

    @Override
    protected Collection<Card> delegate() {
        return delegate;
    }
}

I should add though that your code is very readable and understandable, so don't think of this as a "your code is crap" review. Think about this as a "Have you thought about this?" / "What about this way?" review.

\$\endgroup\$

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.