Redundant code in composition class C++

Question 1

The best answer here is: it depends :) I'll try to clarify it a little, though.

The first question is: does the Player class have any inner logic? If it's a simple container for Hand, I'd simply write Player.GetHand().AddCard(), because there is no reason to duplicate the code inside Player.AddCard() method, and the problem is solved.

Let's suppose now, that there is a need for implementing additional logic for adding a card to Player's hand. That means, that additional code in Player class has to be called while adding a card to Hand. In such case, I see three possible solutions.

(Sources only for demonstration purposes, may not compile)

Restrict access to Hand, such that no one can retrieve it from Player. Player would have to implement methods like AddToHand, RemoveFromHand etc. Doable, but not comfortable to use.
```
class Player
{
private:
    Hand hand;

public:
    void AddToHand(Card & card) 
    { 
        hand.Add(card); 
    }
};
```

Use the observer pattern. When user (class user) calls Player.GetHand().AddCard(), Hand notifies Player, that data has changed and Player can act accordingly. You can achieve this quite easily using std::function from C++11 to implement events.

class Deck
{
private:
    std::function<void(void)> cardsChanged;

public:
    void Add(Card card)
    {
        // Add a card
        if (!(cardsChanged._Empty()))
            cardsChanged();
    }

    void SetCardsChangedHandler(std::function<void(void)> newHandler)
    {
        cardsChanged = newHandler;
    }
};

// (...)

class Player
{
private:
    Hand hand;

    void CardsChanged() { ... }
(...)
public:
    Player()
    {
        hand.SetCardsChangedHandler([&this]() { this.CardsChanged(); } );               
    }
};

Define IHand interface with all necessary interface methods. Hand should obviously implement IHand and Player.GetHand() should return IHand. The trick is, that the IHand returned by Player do not necessarily have to be a Hand instance, but instead it can be a decorator acting as a bridge between user and real Hand instance (see decorator pattern).

class IHand
{
public:
    virtual void Add(Card card) = 0;
    virtual void Remove(Card card) = 0;
};

class Hand : public IHand
{ 
    // Implementations
}

class PlayersHand : public IHand
{
private:
    Hand & hand;
    Player & player;

public:
    PlayersHand(Hand & newHand, Player & newPlayer)
    {
        hand = newHand;
        player = newPlayer;
    }

    void Add(Card card)
    {
        hand.Add(card);
        player.HandChanged();
    }

    // ...
};

class Player
{
private:
    Hand hand;
    PlayersHand * playersHand;

public:
    Player()
    {
        playersHand = new PlayersHand(hand, this);
    }

    IHand GetHand()
    {
        return playersHand;
    }
}

Personally, In the second case, I would choose the second solution - it's quite straightforward and easy to extend and reuse in case of further needs.

Question 2

Function call forwarding is a common practice. You should think about it as adding some level of abstraction. This is not exactly doing the same thing again (which redundancy would mean), but implementing one method, using another one.

You can imagine some modifications in the future, like adding Player's cards cache, or some other stuff that need to be updated when user call addCardToHand. Where would you add the cache-updating code if you didn't implement the forwarding method?

Also note, that the "interface" of Player::addCardToHand doesn't need to be identical with Card::addCard i.e. arguments and returned value can be different in these functions. Maybe in this case it's not so important, but generally the forwarding function is the place where some translation between Player's interface and Hand's interface may be added.