3
\$\begingroup\$

Context

Was inspired from this LeetCode post to do my own System Design exercise of the common Snake and Ladders problem. As I was writing the code Mario Party kept coming to mind so I added a little fun theme to it.

Feedback

Did I make the appropriate system design choices in terms of the classes I've implemented the functionality expressed within them? Let me know of any improvements I can make to the implementation of inheritance and polymorphism with my Space Abstract Class. I have Smart Pointers in place for possible future requirements to take this board from a linear data structure to something you would see in an actual Mario Party game. Any other general feedback would be appreciated too.

Code

#include <cstdlib>
#include <cmath>
#include <iostream>
#include <map>
#include <memory>
#include <string>
#include <vector>

// stars ✩ space ○

class Player
{
    private:
        int m_current_space = 1;
    public:
        Player() {}
        void role_dice() {
            m_current_space += floor( (rand()%10 + 1) / 3 );
        }
        int const get_current_space() {
            if(m_current_space > 9) {
                return 9;
            }
            return m_current_space;
        }
        void set_current_space(int current_space) {
            m_current_space = current_space;
        }
};

class Space
{
    protected:
        int m_id;
        std::vector<std::shared_ptr<Space>> m_paths;
    public:
        Space() {} // requied to use [] operator in map
        Space(int id) : m_id(id) {}
        /*
            POSSIBLE SEG FAUL HERE, BE CARFUL
        */
        void add_path(std::shared_ptr<Space> s) {
            m_paths.push_back(s);
        }
        int get_id() {
            return m_id;
        }
        virtual void event(Player& p) = 0;
        virtual std::string class_type() = 0;
};
class Empty : public Space
{
    public:
        Empty(int id) : Space(id) {}
        void event(Player& p) {}
        std::string class_type() {
            return "Empty";
        }
};
class Ladder : public Space
{
    public:
        Ladder(int id) : Space(id) {}
        virtual void event(Player& p) {
            std::cout << "Waha!" << '\n';
            p.set_current_space(5);
        }
        std::string class_type() {
            return "Ladder";
        }
};
class Snake : public Space
{
    public:
        Snake(int id) : Space(id) {}
        virtual void event(Player& p) {
            std::cout << "Mumba Mia!" << '\n';
            p.set_current_space(4);
        }
        std::string class_type() {
            return "Snake";
        }
};

class Board
{
    private:
        std::map<int, std::shared_ptr<Space>> m_board;
    public:
        void add_space(std::shared_ptr<Space> s) {
            m_board[s->get_id()] = s;
        }
        void draw_board(int position) {
            int i = 1;
            std::string line = "\n";
            for(auto const& space : m_board) {
                if(i%3 == 0) { line = "\n"; }
                else { line = " "; }
                if (space.first == position) {
                    std::cout << "●" + line;
                }
                else if(space.second->class_type() == "Snake" ||
                        space.second->class_type() == "Ladder") {

                    std::cout << "\x1B[32m○\033[0m" + line;
                }
                else { std::cout << "○" + line; }
                ++i;
            }
        }
        const std::map<int, std::shared_ptr<Space>> get_board() {
            return m_board;
        }
        friend std::ostream &operator<<(std::ostream& os, const Board& b) {
            return os;
        }
};

class GameStateManager
{
    private:
        std::string m_state = "game over";
        bool m_playing = false;
    public:
        std::string const get_state() {
            return m_state;
        }
        void set_state(std::string state) {
            m_state = state;
        }
};


int main()
{
    std::cout << "Welcome to Bowser's 9 board game\n";
    std::cout << "Start? y(yes) n(no)\n";
    char wants_to_play;
    std::cin >> wants_to_play;

    if (wants_to_play == 'y' || wants_to_play == 'Y') {
        GameStateManager game_manager;
        game_manager.set_state("playing");
        std::cout << "Let's a go!\nPress Enter to role.\n";
        std::cin.get();

        auto space1 = std::make_shared<Empty>(1);
        auto space2 = std::make_shared<Empty>(2);
        auto space3 = std::make_shared<Ladder>(3);
        auto space4 = std::make_shared<Empty>(4);
        auto space5 = std::make_shared<Empty>(5);
        auto space6 = std::make_shared<Empty>(6);
        auto space7 = std::make_shared<Snake>(7);
        auto space8 = std::make_shared<Empty>(8);
        auto space9 = std::make_shared<Empty>(9);

        std::vector<std::shared_ptr<Space>> spaces {
            space1, space2, space3,
            space4, space5, space6,
            space7, space8, space9
        };

        Board bowsers_bigbad_laddersnake;
        for(auto space : spaces) {
            bowsers_bigbad_laddersnake.add_space(space);
        }

        Player mario;
        bowsers_bigbad_laddersnake.draw_board(mario.get_current_space());

        int turn = 0;
        while(game_manager.get_state() == "playing") {
            std::cin.get();
            std::cout << "-- Turn " << ++turn << " --" << '\n';

            mario.role_dice();
            auto board = bowsers_bigbad_laddersnake.get_board();
            std::shared_ptr<Space> s = board[mario.get_current_space()];
            s->event(mario);
            bowsers_bigbad_laddersnake.draw_board(mario.get_current_space());

            std::cout << mario.get_current_space() << '\n';
            if (mario.get_current_space() >= 9) {
                game_manager.set_state("game over");
            }
        }

        std::cout << "Thanks a so much for to playing!\nPress any key to continue . . .\n";
        std::cin.get();

    } else if (wants_to_play == 'n' || wants_to_play == 'N') {
        std::cout << "Thanks a so much for to playing!\nPress any key to continue . . .\n";
        std::cin.get();
    }

    return 0;
}
\$\endgroup\$
3
\$\begingroup\$
  • This has been raised in many of your reviews, but let me repeat: make member functions that don't modify object contents const. This improves readability and advocates correctness. (One example is get_board(), but there are plenty more).

  • Don't pass expensive objects (like std::string) by value unless you have to. A better idea is to pass them by const-ref (e.g., const std::string& state).

  • A member function like class_type() is a bad idea. Don't do this, it only breaks abstraction. As it stands, someone queries the objects for their type and does an action (= drawing) accordingly. Instead, make every object know how it should be drawn. For example, drop the class_type() function and replace it with something like std::string draw() that returns whatever is correct. Then your board just implements the logic of "for every object o, call o.draw()" and your code is greatly simplified.

  • It seems that GameStateManager is completely useless. You can replace it with a boolean in the main program and simplify your program.

  • Consider using const or constexpr values for all the colors and other strings you are printing.

  • The declaration of space1 through space9 is questionable. In fact, whenever you feel like writing something like that, you should start questioning yourself and think what the better alternative is. In this case, there's no need to name these variables. You can directly initialize the vector by constructing the objects in-place in a constructor call.

  • Is there a reason the Board object must be copied inside the main program on the line auto board = bowsers_bigbad_laddersnake.get_board();?

\$\endgroup\$
  • \$\begingroup\$ Love the optimization to have the classes manage how they draw/render themselves on board when board calls its draw method. Is there a proper way to determine the class type of a derived class other than the hard coded names I implemented in class_type()? \$\endgroup\$ – greg Apr 23 at 20:51
  • \$\begingroup\$ @greg dynamic_cast, but you should try not to find out the types. \$\endgroup\$ – Juho Apr 24 at 7:08
2
\$\begingroup\$

When you roll the die (not role), the result is one of:

0 1 2 3 4 5 6 7 8 9   rand() % 10
1 2 3 4 5 6 7 8 9 10  + 1
0 0 1 1 1 2 2 2 3 3   / 3

Rolling a 0 is not fun. Whenever I roll the die, something should happen.


When I read the code from top to bottom, I wondered what the magic number 9 had to do with the player. Further down in the code I learned that the board is restricted to 9 spaces. This number 9 should not appear in the Player class, but only in the Board class. And even the it should not be the literal 9, but spaces.size().


In draw_board it is inconsistent to write the escape code once as \x1B and once as \033. As a benefit for the readers who don't know the color table by heart, you should explain what these color codes do:

std::string green = "\x1B[32m";
std::string normal = "\x1B[0m";

std::cout << green << "○" << normal << "\n";

I don't understand the initial question "do you want to play"? — of course I want, otherwise I wouldn't have started the game at all.


Why did you choose the really long variable name for the board, why not just Board board? As it is written now, the variable name draws too much attention.

\$\endgroup\$
  • \$\begingroup\$ >Rolling a 0 is not fun \$\endgroup\$ – WooWapDaBug Apr 23 at 14:43
  • \$\begingroup\$ For your normal color string why are you using std::string normal = "\x1B[0m"; instead of \033[0m"? \$\endgroup\$ – greg Apr 23 at 20:46
  • \$\begingroup\$ Also fixed the rolling and add all of the other feedback here leetcode.com/discuss/interview-question/system-design/159188/…. This was a fun exercise! \$\endgroup\$ – greg Apr 23 at 22:27
  • 1
    \$\begingroup\$ @greg I'm using \x1B instead of 033 because I don't like octal numbers. They are a thing of the 1970s and I don't want to promote them. I'd even prefer them to be banned from programming languages as far as possible. (Probably I'm not involved enough in embedded or microcontrollers or retro computing to appreciate their value.) It's mainly a personal choice. \$\endgroup\$ – Roland Illig Apr 23 at 22:32
  • 1
    \$\begingroup\$ @greg the main annoyance about octal numbers is that in several programming languages the integer literal 0100 does not mean one hundred but instead sixty-four. This is so surprising that probably every programmer wonders what is wrong with their code. That's unnecessary. If octal literals were written as 0o100 instead, I would not be opposed to them since one does not accidentally type that. \$\endgroup\$ – Roland Illig Apr 24 at 16:14

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.