Trading Card Game's Hand and HandView implementation using composition and decoration

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.

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 {
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 iterator();

@Override
public Spliterator spliterator();

@Override
public int size();

@Override
public void forEach(final Consumer 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();

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

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 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.

@Override
public String toString();

@Override
public Iterator iterator();

@Override
public Spliterator spliterator();

@Override
public int size();

@Override
public void forEach(final Consumer 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, 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 {
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 implements Hand {
private final List list = new ArrayList<>();
private final int capacity;

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

@Override
p

StackExchange Code Review Q#48466, answer score: 4