3
\$\begingroup\$

I'm writing game in SFML and I got question about class design. For example, I have a dice class that looks like this:

#include <SFML/Graphics.hpp>
#include <SFML/Audio.hpp>
#include "DEFINITIONS.h"
#include <random>
#include <chrono>

class Dice {
    std::default_random_engine rng;
    sf::Texture m_texture;
    sf::RectangleShape m_dice;
    sf::SoundBuffer m_buffer;
    sf::Sound m_sound;
public:
    Dice(float positionX, float positionY);
    ~Dice() = default;
    int roll();
    inline sf::RectangleShape& getShape() { return m_dice; }
    inline void setTexture(std::string filePath) { m_texture.loadFromFile(filePath); }
};   

Dice::Dice(float positionX, float positionY) {
    m_dice.setSize(sf::Vector2f(DICE_WIDTH, DICE_HEIGHT));
    m_dice.setPosition(positionX, positionY);
    m_texture.loadFromFile(DICE_DEFAULT_TEXTURE_FILEPATH);
    m_dice.setTexture(&m_texture);
    m_buffer.loadFromFile(DICE_SOUND_FILEPATH);
    m_sound.setBuffer(m_buffer);
}

int Dice::roll() {
    m_sound.play();

    rng.seed(std::chrono::system_clock::now().time_since_epoch().count());
    std::uniform_int_distribution<int> dist(1, 6);
    int number = dist(rng);

    std::string filePath = "resources//dice" + std::to_string(number) + ".png";
    m_texture.loadFromFile(filePath);
    m_dice.setTexture(&m_texture);

    return number;
}

And few other classes are designed in a similar way. For example, Button has sf::Texture which depends on the bool enabled; variable. A colleague of mine, who has a bit more experience with programming, told me that he isn't sure about that solution. In his opinion it's better to split classes into pure logic and pure view, for example DiceLogic and DiceView, or at least put logic and view in separate methods and then call them in another one. What's the best solution here to maintain clean code?

\$\endgroup\$
5
\$\begingroup\$

I see some things that may help you improve your code and offer some suggestions regarding your main question.

Think carefully about class design

The main question you've asked here is about class design. It often does make sense to separate the logic from the presentation of the data. Reasons for doing so include:

  1. simplifies any later adaptation (e.g. porting to some non-SFML platform)
  2. makes automated testing simpler
  3. may be simpler to understand

On the other hand:

  1. YAGNI
  2. this is a really small class
  3. may complicate understanding with proliferation of classes

I could argue it either way for this one, but I would offer this suggestion: if you leave it as a single class, at least separate the operation from the presentation. That is, roll could return the appropriate int but I'd suggest deriving Dice from sf::Drawable and implementing a separate draw() function. Generally, I'd lean in the direction of having separate classes for the logic and presentation, even with such a small class. I find that it often helps with maintenance.

Seed the random number generator exactly once

The existing class reseeds the random number generator each time roll is called. Not only does that reduce the randomness, but the seed is not particularly random, either. Better might be to use std::random_device if your compiler (and hardware) supports it.

Think carefully about naming

The class is called Dice but "dice" is the plural of "die" and this class actually appears to implement a single die. I'm not going to strongly recommend changing it to Die (because that's also a verb which might confuse someone) but it's worth thinking about.

Understand the use of inline

The inline keyword is often misconstrued as something that will enhance the performance of the compiled code. Typically, it won't. If you're using it because you want to define the functions in the header, I'd suggest that you are probably better off splitting the implementation and the interface into .cpp and .h files instead. See this answer for details.

Handle errors

Right now, the code doesn't check for errors when attempting to load resource files. It should, and should provide some reasonable behavior if anything fails.

\$\endgroup\$
  • \$\begingroup\$ Thank you for hints, I'm trying to improve my code all the time but after all I'm still a newbie that starts everyday coding from refactoring yesterday's work. Well I must admit, it was easier when I tried to make my projects in console, I was just focused on current task and class design was quite easy. Once I started using SFML everything started to get confusing. Now I'm thinking if I shouldn't just focus entirely on logic first and then start writing presentation side? Or then, when I'll try to connect those two it will become really painfull? \$\endgroup\$ – JohnDoe Jan 3 '18 at 22:24
  • \$\begingroup\$ Most of us are still working on improving our code -- that never ends! Looks to me that you're doing fine now. Write something that works and then try to refine/refactor it. If you can also get into the habit of writing test code to go with the code you write, you can refactor with higher confidence. \$\endgroup\$ – Edward Jan 3 '18 at 22:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.