Data Model for a Memory Game

Question

I have a Memory style card matching game. The project is big enough that I want to break up the review. The first part that I am giving you is the data model. If you are interested the full project can be found on github.

The first file I am sharing holds all of my constexpr that I use to eliminate magic numbers. It also holds a static PRNG and my enums. I don't know if this is considered acceptable practice so I would appreciate a comment in regards to what I did here.

Expressions.h

#ifndef BRUGLESCO_MEMORY_EXPRESSIONS_H
#define BRUGLESCO_MEMORY_EXPRESSIONS_H

#include <random>

static std::random_device rd;
static std::mt19937 generator(rd());

constexpr unsigned screen_width = 1500u;
constexpr unsigned screen_height = 800u;

constexpr float menu_button_width = 256.f;
constexpr float menu_button_height = 256.f;

constexpr float play_button_x = 622.f;
constexpr float play_button_y = 96.f;
constexpr float six_button_x = 95.f;
constexpr float eight_button_x = 446.f;
constexpr float twelve_button_x = 797.f;
constexpr float sixteen_button_x = 1148.f;
constexpr float pair_button_y = 448.f;

constexpr float play_string_x_offset = 18.f;
constexpr float play_string_y_offset = 28.f;
constexpr float six_string_x_offset = 80.f;
constexpr float eight_string_x_offset = 40.f;
constexpr float twelve_string_x_offset = 15.f;
constexpr float sixteen_string_x_offset = 8.f;
constexpr float pair_string_x_offset = 40.f;
constexpr float pair_string_y_offset = 100.f;

constexpr float game_button_width = 128.f;
constexpr float game_button_height = 64.f;

constexpr float pause_x = 1244.f;
constexpr float reset_x = 1372.f;
constexpr float pause_offset = 12.f;
constexpr float reset_offset = 5.f;

constexpr float display_x = 1244.f;
constexpr float player_one_display_y = 64.f;
constexpr float player_two_display_y = 432.f;
constexpr float display_offset = 20.f;

constexpr float display_width = 256.f;
constexpr float display_height = 368.f;

constexpr float canvas_width = screen_width - display_width;
constexpr float canvas_height = static_cast<float>(screen_height);

constexpr float player_string_x = 1265.f;
constexpr float player_one_string_y = 72.f;
constexpr float player_two_string_y = 440.f;

constexpr float player_card_x = 1265.f;
constexpr float player_card_x_offset = 15.f;
constexpr float player_one_card_y = 120.f;
constexpr float player_two_card_y = 488.f;

constexpr float card_width = 128.f;
constexpr float card_height = 128.f;

constexpr unsigned max_pairs = 16u;

constexpr float win_width = screen_width - 300.f;
constexpr float win_height = screen_height - 150.f;
constexpr float win_x = 150.f;
constexpr float win_y = 75.f;
constexpr float winstring_x = 400.f;
constexpr float winstring_y = 100.f;
constexpr float player_winstring_x = winstring_x - 50.f;
constexpr float player_winstring_y = winstring_y + 200.f;

enum class DeckSize
{
    six = 6,
    eight = 8,
    twelve = 12,
    sixteen = 16
};

enum class CardState
{
    unmatched,
    checking,
    matched
};

enum class menuMouseIn
{
    play,
    six,
    eight,
    twelve,
    sixteen,
    none
};

enum class gameMouseIn
{
    pause,
    reset,
    none
};

enum class winState
{
    none,
    draw,
    playerOne,
    playerTwo
};

#endif // !BRUGLESCO_MEMORY_EXPRESSIONS_H

---

One of the concepts I used in designing this project was the idea of Top-Down Design. I welcome any feedback on this approach as a whole and its fit in a project like mine as well as my implementation of it. As this was my approach in building the project I will also present it to you in this manner.

Main.cpp

#include "Memory.h"

int main()
{
    Memory memory;
    memory.run();
}

---

Memory is the name of the class that is essentially the entire application.

Memory.h

#ifndef BRUGLESCO_MEMORY_MEMORY_H
#define BRUGLESCO_MEMORY_MEMORY_H

#include "Expressions.h"
#include "ModelData.h"
#include "Viewport.h"

#include <SFML\Graphics.hpp>

class Memory
{
public:
    void run();
private:
    ModelData data;
    Viewport view{ data };
    bool playing{ true };

    void input();

    void update();

    void draw();
};

#endif // !BRUGLESCO_MEMORY_MEMORY_H

Memory.cpp

#include "Memory.h"

void Memory::run()
{
    while (playing)
    {
        input();
        update();
        draw();
    }
}

void Memory::input()
{
    view.input();
}

void Memory::update()
{
    playing = view.isOpen();
    data.update();
    view.update();
}

void Memory::draw()
{
    view.draw();
}

---

In the above section the only part that I am going to continue to present is the ModelData class. The Viewport contains the presentation logic and I am reserving that for a future question.

ModelData.h

#ifndef BRUGLESCO_MEMORY_MODELDATA_H
#define BRUGLESCO_MEMORY_MODELDATA_H

#include "Deck.h"
#include "Expressions.h"
#include "Player.h"

class ModelData
{
public:
    bool isMenu() const;

    bool isOver() const;

    void setSize(const DeckSize& size);

    void resetTurnCards();

    std::vector<Card>& getDeck();

    std::vector<unsigned>& getFailedCards();

    std::vector<unsigned>& getMatchedCards();

    Player* getPlayer();

    bool playerOneTurn() const;

    void play();

    void quit();

    void update();
private:
    bool menu{ true };
    unsigned numPlayers{ 2 };
    Player playerOne{ 1 };
    Player playerTwo{ 2 };
    Player* activePlayer{ &playerOne };
    Deck deck;
    DeckSize deckSize{ DeckSize::six };
    winState winner{ winState::none };
    bool p1Turn{ true };
    bool ended{ false };

    void dealDeck();
};

#endif // !BRUGLESCO_MEMORY_MODELDATA_H

---

ModelData.cpp

#include "ModelData.h"

bool ModelData::isMenu() const
{
    return menu;
}

bool ModelData::isOver() const
{
    return ended;
}

void ModelData::setSize(const DeckSize& size)
{
    deckSize = size;
}

void ModelData::resetTurnCards()
{
    deck.reset();
}

std::vector<Card>& ModelData::getDeck()
{
    return deck.getCards();
}

std::vector<unsigned>& ModelData::getFailedCards()
{
    return deck.getFailedCards();
}

std::vector<unsigned>& ModelData::getMatchedCards()
{
    return deck.getMatchedCards();
}

Player* ModelData::getPlayer()
{
    return activePlayer;
}

bool ModelData::playerOneTurn() const
{
    return p1Turn;
}

void ModelData::play()
{
    dealDeck();
    menu = false;
}

void ModelData::quit()
{
    playerOne.reset();
    playerTwo.reset();
    activePlayer = &playerOne;
    deck.clear();
    menu = true;
    p1Turn = true;
    ended = false;
    winner = winState::none;
}

void ModelData::update()
{
    CardState deckState = deck.checkCards();
    if (deck.checkWin() && !ended)
    {
        if (playerOne.getScore() == playerTwo.getScore())
        {
            ended = true;
            winner = winState::draw;

        }
        else if (playerOne.getScore() > playerTwo.getScore())
        {
            ended = true;
            winner = winState::playerOne;
        }
        else
        {
            ended = true;
            winner = winState::playerTwo;
        }
    }
    else if (deckState == CardState::matched)
    {
        // player takes card
        activePlayer->scorePoint();
    }
    else if (deckState == CardState::unmatched)
    {
        if (p1Turn)
        {
            activePlayer = &playerTwo;
        }
        else
        {
            activePlayer = &playerOne;
        }
        p1Turn = !p1Turn;
    }
}

void ModelData::dealDeck()
{
    deck.set(deckSize);
}

---

The ModelData class has quite a few getters for the Viewport to collect information to display. Should I prefer a friend class here? As a rule I try to avoid friend classes because I worry that I am going to break encapsulation that way but having getters and setters can be an anti-pattern in C++ too? Or is this the simple and correct solution? This class also uses the player class and the Deck class which I will also provide.

Player.h

#ifndef BRUGLESCO_MEMORY_PLAYER_H
#define BRUGLESCO_MEMORY_PLAYER_H

#include "Expressions.h"

class Player
{
public:
    Player(unsigned identity);

    void scorePoint();

    void reset();

    unsigned getIdentity() const;

    unsigned getScore() const;
private:
    unsigned identity;
    unsigned score{ 0 };
};

#endif // !BRUGLESCO_MEMORY_PLAYER_H

---

Player.cpp

#include "Player.h"

Player::Player(unsigned identity) :
    identity{ identity }
{
}

void Player::scorePoint()
{
    ++score;
}

void Player::reset()
{
    score = 0;
}

unsigned Player::getIdentity() const
{
    return identity;
}

unsigned Player::getScore() const
{
    return score;
}

---

Lastly is the Deck that holds the Cards.

Deck.h

#ifndef BRUGLESCO_MEMORY_DECK_H
#define BRUGLESCO_MEMORY_DECK_H

#include "Card.h"
#include "Expressions.h"
#include "Player.h"

#include <vector>

class Deck
{
public:
    void set(const DeckSize& deckSize);

    void reset();

    CardState checkCards();

    bool checkWin();

    std::vector<Card>& getCards();

    std::vector<unsigned>& getFailedCards();

    std::vector<unsigned>& getMatchedCards();

    void clear();
private:
    std::vector<Card> deck;
    std::vector<unsigned> upCards;
    std::vector<unsigned> matchedCards;
};

#endif // !BRUGLESCO_MEMORY_DECK_H

---

Deck.cpp

#include "Deck.h"

#include <algorithm>

void Deck::set(const DeckSize& deckSize)
{
    deck.clear();
    unsigned k = 0;
    for (unsigned i = 0; i < static_cast<unsigned>(deckSize); ++i)
    {
        deck.push_back(Card(i));
        ++k;
        deck.push_back(Card(i));
        ++k;
    }
    std::shuffle(std::begin(deck), std::end(deck), generator);
}

void Deck::reset()
{
    upCards.clear();
    matchedCards.clear();
}

CardState Deck::checkCards()
{
    unsigned count = 0;
    std::vector<Card*> cards;
    std::vector<unsigned> indices;
    for (unsigned i = 0; i < deck.size(); ++i)
    {
        if (deck[i].checkState() == CardState::checking)
        {
            ++count;
            cards.push_back(&deck[i]);
            indices.push_back(i);
        }
    }

    if (count == 2)
    {
        if (cards[0]->getMatch() == cards[1]->getMatch())
        {
            matchedCards = indices;
            cards[0]->match();
            cards[1]->match();
            return CardState::matched;
        }
        else
        {
            upCards = indices;
            cards[0]->reset();
            cards[1]->reset();
            return CardState::unmatched;
        }
    }
    else
    {
        return CardState::checking;
    }
}

bool Deck::checkWin()
{
    for (std::vector<Card>::iterator card = deck.begin(); card != deck.end(); ++card)
    {
        if (card->checkState() != CardState::matched)
        {
            return false;
        }
    }
    return true;
}

std::vector<Card>& Deck::getCards()
{
    return deck;
}

std::vector<unsigned>& Deck::getFailedCards()
{
    return upCards;
}

std::vector<unsigned>& Deck::getMatchedCards()
{
    return matchedCards;
}

void Deck::clear()
{
    deck.clear();
}

---

Card.h

#ifndef BRUGLESCO_MEMORY_CARD_H
#define BRUGLESCO_MEMORY_CARD_H

#include "Expressions.h"

class Card
{
public:
    Card(unsigned matchNum);

    unsigned getMatch() const;

    CardState checkState() const;

    void flip();

    void match();

    void reset();
private:
    unsigned matchNum;
    CardState cardState{ CardState::unmatched };
    bool faceUp{ false };
};

#endif // !BRUGLESCO_MEMORY_CARD_H

---

Card.cpp

#include "Card.h"

Card::Card(unsigned matchNum) :
    matchNum{ matchNum }
{}

unsigned Card::getMatch() const
{
    return matchNum;
}

CardState Card::checkState() const
{
    return cardState;
}

void Card::flip()
{
    if (cardState == CardState::unmatched)
    {
        cardState = CardState::checking;
        faceUp = true;
    }
}

void Card::match()
{
    cardState = CardState::matched;
}

void Card::reset()
{
    cardState = CardState::unmatched;
    faceUp = false;
}

---

I expressed some of my concerns throughout the question but will list them and others here as well:

• Any and all aspects which I happily embrace. I'm an amateur looking to improve and this is the only way to get feedback on things I didn't even know were issues.
• Top-down design, DRY, SOLID and other basic software techniques. This has been a bit of a struggle for me and improving on it has been the major goal of this project so I would love input about how I am doing.
• Extensibility and Maintainability. I would love feedback on how well I applied this. I realized after the project was done that hard-coding 2-players was not very extensible and changing it in the Model would be easy to do but changing it in the View (separate) would require a bit more work. I feel like I failed there but I also have no desire to add a feature that allows more than 2 players so I'm not rewriting it now (although I wish I had written it that way to begin with.) Still I welcome your input.
• Clean Code, Naming, Readability and conventions. I am trying to learn to write with the understanding of YANA. Readable code is important but being self-taught leads to little feedback in this regard which makes this site such a wonderful resource to me.

Answer 1

Design review or code review?

Well, in real-world projects we communicate the design in its own way first, not just say “here’s code that implements my design.” I would like to see a paragraph explaining how it works in very simple overview, and a sketch showing the different types and their relationships (UML pseudo-code).

Expressions.h

You might put the whole thing into your own namespace.

I always wonder why people use float when it’s not for some huge matrix or something. double is the native floating point type. Unless you are using SIMD, they are just as fast.

For the enums, you might want to always put none at the top, so default initialization is the most useful or natural meaning.

main

I see you did not cram everything into main, but simply use this to drive other functions. That is more realistic for code that goes into a large project.

Memory.h

It is generally good to order your #include lines: standard library first, low-level utilities next, specialized libraries, and then the project’s own specific code.

ModelData.h

Double-spacing your members means I have to scroll the window to read the class. What is the point of that?

I see you use default initializers and don’t write a constructor. Rule Of Zero. But, activePlayer is a pointer to another member in the same object. I think that will not work properly with the automatically supplied copy constructor and assignment operator.

ModelData.cpp

A lot of the functions I see at the top could simply be inline in the header. return ended takes up a lot of space for such a trivial accessor!

   if (playerOne.getScore() == playerTwo.getScore())
    {
        ended = true;
        winner = winState::draw;

    }
    else if (playerOne.getScore() > playerTwo.getScore())
    {
        ended = true;
        winner = winState::playerOne;
    }
    else
    {
        ended = true;
        winner = winState::playerTwo;
    }

You duplicate the same thing on each branch, and the other statement is just different assignments to the same thing. How about:

    ended = true;
    winner = choose_winner();

Remember, It’s top-down design decomposition. You are doing one thing, and it takes a bunch of lines, and all of it uses the same small subset of data. So it should be a separate function.

I do think that figuring the winner enum should be able to be done in a more elegant manner, in any case.

   if (p1Turn)
    {
        activePlayer = &playerTwo;
    }
    else
    {
        activePlayer = &playerOne;
    }

try:

    activePlayer = p1Turn? &playerTwo : &playerOne;

or better yet, do the next line (p1Turn = !p1Turn) first, so the names match up with the condition.

Data model: It seems awkward that you have p1Turn and a pointer both doing the same thing. winner has its own enum rather than pointing to the winning player or nullptr. Most of your code is in matching up different representations of the same internal meaning.

Having two separately named variables for the players is making more work. Everywhere you need an if statement to choose between them, or duplicated code. Always use an indexed collection instead of separate names. So make

    Player player[2]  { 1, 2 };

You have numPlayers which can’t change with your fixed names, so why do you need that? All your code is referring directly to one or the other so you don’t loop over n players anyway. You did not make it constant.

The current player is stored internally as the index. When you want a pointer to the player object, call the accessor which looks it up using the index — don’t store the pointer separately too! Follow me?

Deck.cpp

std::vector<Card*> cards;

draws my attention: storing raw pointers? What are you doing with them. So looking closer at the code,

for (unsigned i = 0; i < deck.size(); ++i)
{
    if (deck[i].checkState() == CardState::checking)
    {
        ++count;
        cards.push_back(&deck[i]);
        indices.push_back(i);
    }
}

You should use a ranged for loop and avoid looping by index like this.

for (auto& card : deck)
{
    if (card.checkState() == CardState::checking)
    {
        ++count;
        cards.push_back(&card);
        indices.push_back(i);  // ?????
    }
}

I saw the indecies vectors and wondered what the unsigned values were for. It is hard to evaluate the code without knowing the data model via an explaination. I wonder if using index numbers are a good solution for keeping track of cards. Why not pointers to the card instance, for example?

There is a Boost library for multi-index collection that can be handy if you are keeping multiple ways to look up the same data.

---

Good luck!!