trjhtr

I asked for feedback on my Dice class and I was suggested that I should separate logic from view. I decided to completely refactor my project and I'd like to share a small piece of it (the Dice class and connection between logic and view) and ask for feedback again.

So lets start with logic::Dice

namespace logic 
 class Dice 
 std::default_random_engine rng;
 int m_currentNumber;
 public:
 Dice() 
 rng.seed(static_cast<int>(std::chrono::system_clock::now().time_since_epoch().count()));
 
 ~Dice() = default;
 void rollNewNumber();
 int getCurrentNumber() const;
 ;


void logic::Dice::rollNewNumber() 
 std::uniform_int_distribution<int> dist(1, 6); 
 m_currentNumber = dist(rng); 


int logic::Dice::getCurrentNumber() const 
 return m_currentNumber;

Now view::Dice

 namespace view 
 class Dice 
 sf::Texture m_texture;
 sf::RectangleShape m_shape;
 sf::SoundBuffer m_buffer;
 sf::Sound m_sound;
 public:
 Dice();
 ~Dice() = default;
 sf::RectangleShape& get(); 
 void changeTexture(int);
 void playSound();
 ;


view::Dice::Dice() 
 m_shape.setSize(sf::Vector2f(DICE_WIDTH, DICE_HEIGHT));

 if (!m_texture.loadFromFile(DICE_TEXTURE)) std::cout << "Can't load texture.n"; 
 m_shape.setTexture(&m_texture);
 m_shape.setTextureRect(sf::IntRect(0, 0, 55, 55));

 if (!m_buffer.loadFromFile(DICE_SOUND)) std::cout << "Can't load sound.n";
 m_sound.setBuffer(m_buffer);


sf::RectangleShape& view::Dice::get() 
 return m_shape;


void view::Dice::changeTexture(int newNumber) 
 switch (newNumber) 
 case 1: 
 m_shape.setTextureRect(sf::IntRect(0, 0, 55, 55));
 break;
 
 case 2: 
 m_shape.setTextureRect(sf::IntRect(55, 0, 55, 55));
 break;
 
 case 3: 
 m_shape.setTextureRect(sf::IntRect(110, 0, 55, 55));
 break;
 
 case 4: 
 m_shape.setTextureRect(sf::IntRect(165, 0, 55, 55));
 break;
 
 case 5: 
 m_shape.setTextureRect(sf::IntRect(220, 0, 55, 55));
 break;
 
 case 6: 
 m_shape.setTextureRect(sf::IntRect(275, 0, 55, 55));
 break;
 
 


void view::Dice::playSound() 
 m_sound.play();

Now to connect logic with presentation, I got 2 classes. view::GameState which derives from view::State that looks like that

namespace view 
 class State 
 public:
 virtual void initialise() = 0;
 virtual void handleUserInput() = 0;
 virtual void update(sf::Time dt) = 0;
 virtual void draw() = 0;
 ;

And most important in this case part of my view::GameState

 class GameState : public State 
 logic::Game m_game;
 view::Dice m_diceOne, m_diceTwo;
 ;

And inside my view::GameState::handleUserInput() I check when "Roll the dice" button is clicked

 if (this->m_data->inputManager.isSpriteClicked(this->m_rollButton, evnt, this->m_data->window)) 
 rollTheDice();
 

Which finnaly gets to connection between logic and presentation

void view::GameState::rollTheDice() 
 m_game.rollTheDice();
 m_diceOne.playSound();
 m_diceOne.changeTexture(m_game.getDiceOne().getCurrentNumber());
 m_diceTwo.changeTexture(m_game.getDiceTwo().getCurrentNumber());

That leads to logic::Game class which is kind of game engine, that does all the logic and has all the logic objects like Dice, Player, GameBoard

 namespace logic 
 class Game 
 logic::Dice m_diceOne, m_diceTwo;

 int m_throwsInCurrentTurn = 0;
 int m_doublesInCurrentTurn = 0; 
 int m_totalRollResult = 0;
 public:
 void rollTheDice();
 logic::Dice& getDiceOne();
 logic::Dice& getDiceTwo();
 ;
 

 void logic::Game::rollTheDice() 
 m_throwsInCurrentTurn++;
 m_diceOne.rollNewNumber();
 m_diceTwo.rollNewNumber();
 int diceOneResult = m_diceOne.getCurrentNumber();
 int diceTwoResult = m_diceTwo.getCurrentNumber();
 m_totalRollResult += diceOneResult + diceTwoResult;
 if (diceOneResult == diceTwoResult) m_doublesInCurrentTurn++;
 

 logic::Dice& logic::Game::getDiceOne() 
 return m_diceOne;
 

 logic::Dice & logic::Game::getDiceTwo() 
 return m_diceTwo;
 

I would appriciate your feedback and suggestions how to improve my code.

[code] m_

One of the main benefits of using the m_ prefix is not having to put this-> in front of members!

[design] overall

While separating logic and presentation are important, I'd suggest it has more to do with ensuring that a class does only one thing (the "Single Responsibility Principle"), rather than with overall organisation.

While the logic and view namespaces might be useful if there were lots of similar classes that needed to be split in exactly this way, I'm not sure they're necessary here. Also, I don't think that the State and GameState classes need to be in either (view::GameState contains both logic and view objects).

Throughout the code, there are many places where data is hard-coded into a class. This generally indicates "unfinished" design / programming, and leads to duplication and missed opportunities for code reuse.

[design] specifics

I'd recommend the following:

• 
  view and logic namespaces - remove them for the reasons mentioned above.


• 
  logic::Dice - rename to just Dice.


• 
  view::Dice -
  
  
  • move the dice-specific hard-coded data definitions outside of the class.
  
  
  • split the sf::Sound member into a separate SoundEffect class.
  
  
  • rename the remainder to TexturedQuad or similar.
  
  


• 
  logic::Game - rename to GameLogic.


• 
  view::State and view::GameState - move out of view namespace (it's hard to say more without seeing full code).

So we end up with the following architecture:

class TexturedQuad 
public:
 TexturedQuad(sf::Vector2 size, std::string const& textureFileName, sf::IntRect const& textureRect);

 void setPosition(sf::Vector2f const& position); // ??
 void setTextureRect(sf::IntRect const& textureRect);
 void draw(); // ??

private:
 sf::Texture m_texture;
 sf::RectangleShape m_shape;
;

class SoundEffect 
public:
 SoundEffect(std::string const& fileName);

 void play();

private:
 sf::Sound m_sound;
 sf::SoundBuffer m_buffer;
;

class State ... ;
class GameState : public State 
 ...

 GameLogic m_game;
 SoundEffect m_diceSoundEffect; // note, we only need one of these!
 TexturedQuad m_diceTexturedQuad; // and (probably) only one of these!
;

class Dice ... ;
class GameLogic 
 ...

 Dice m_dice; // only one of these!

 int m_rollOne = 0; // store the current rolls in the class instead...
 int m_rollTwo = 0;
;

By separating out the data, and passing it into constructors where necessary, we end up with generic classes that can be reused. We've also uncovered some duplication of class instances:

• We only need one dice sound effect instance, not two.


• We only need one textured quad, which can be repositioned and the texture coordinates changed to draw both dice. (Note: I don't know sfml, and am assuming that changing the position and "textureRect" are trivial operations that can be done in the rendering loop. You may still need two quads, if this isn't the case.)


• Unless there's a need to reproduce two separate runs of dice rolls in the random number generator (i.e. use two separate seeds), we only need one dice instance, which we can roll twice.

So the functions can be rewritten as:

void GameState::rollTheDice() 
 m_game.rollTheDice();
 m_diceSoundEffect.play();


void GameState::draw() 

 m_diceTexturedQuad.setTextureRect(getTextureRectFromDiceRoll(m_game.m_rollOne));
 m_diceTexturedQuad.setPosition(...);
 m_diceTexturedQuad.draw();

 m_diceTexturedQuad.setTextureRect(getTextureRectFromDiceRoll(m_game.m_rollTwo));
 m_diceTexturedQuad.setPosition(...);
 m_diceTexturedQuad.draw();

 ...


void GameLogic::rollTheDice() 
 m_throwsInCurrentTurn++;

 m_rollOne = m_dice.roll();
 m_rollTwo = m_dice.roll();

 m_totalRollResult += m_rollOne + m_rollTwo;

 if (m_rollOne == m_rollTwo)
 m_doublesInCurrentTurn++;

and constructor initializer lists used to pass data into our generic classes, e.g.:

GameState::GameState():
 m_game(),
 m_diceSoundEffect(DICE_SOUND),
 m_diceTexturedQuad(sf::Vector2f(DICE_WIDTH, DICE_HEIGHT), DICE_TEXTURE, sf::IntRect(0, 0, 55, 55)) 

[design] dice class

The same principles of removing data from the class allow us to change the dice class so it can be used for a die with any number of sides:

class Dice 
public:

 using rng_t = std::mt19937; // default_random_engine or whatever...
 using output_t = rng_t::result_type;

 Dice(output_t sides, output_t seed = std::random_device()()):
 m_distribution(output_t 1 , sides),
 m_rng(seed) 

 output_t roll() 
 return m_distribution(m_rng);
 

private:
 std::uniform_int_distribution<output_t> m_distribution;
 rng_t m_rng;
;

...

Dice d(6);
std::cout << d.roll() << std::endl;

I believe the standard way to obtain a random seed is to use std::random_device as above.

[naming]

Technically, the word "dice" is a plural of the singular "die". I'd be tempted to rename Dice to Die, as the class represents only one of the things. I'd expect a "Dice" class to provide an interface for multiple dice, such as 2d6 or 3d4 rolls, etc.

But others might disagree... :S