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.