You follow good practices for inheritance

The "example" you show at the beginning uses some bad practices:

• It doesn't make the destructor of MapSite virtual
• It doesn't use override in the derived classes

But in your implementation, you are doing the correct thing, which is great!

Use of std::shared_ptr

I implemented it using smart pointers. Because of the cyclic structure of rooms and doors, I think I have to use shared_ptr and weak_ptr. I have only used unique_ptr before in my projects.

While that might at first sound like a good idea, it's not a great solution. std::shared_ptr is very safe, but it has issues:

• It's not as efficient as std::unique_ptr.
• It can result in memory leaks if you have std::shared_ptrs pointing to each other.

You narrowly avoid the latter because you only connect Rooms together via Doors, and Doors use std::weak_ptrs. But if you did:

r1->setSide(east, r2);
r2->setSide(west, r1);

Then you have created a cycle, and the memory for r1 and r2 would not be freed automatically anymore if you just deleted r1 and r2.

But in your code, you actually don't need std::shared_ptr. You already have Maze storing pointers to all the map sites, so you could have Maze be the sole owner of them. As long as you guarantee that Maze will never delete a map site that is still in use, you can safely use raw pointers in Room and Door.

Simplify adding rooms

I see in createMaze() that you still have to do several manual steps to add rooms to the maze. First of all, you have to manually create a room, and then in a separate step add it to the maze. That's more work, and leaves the possibility of forgetting the second step. Ideally, you have one function to do all that for you.

One possibilty would be to add a function to Maze to do this:

class Maze {
public:
    Room& addRoom(int id) {
        return rooms.emplace_back(id);
    }

private:
    std::vector<Room> rooms;
};

Then you can write:

MazeGame createMaze() {
  Maze maze;
  auto r1 = maze.addRoom(1);
  auto r2 = maze.addRoom(2);

  r1.setSide(north, …);
  …
}

Of course, you can't just do this for Room, you have to do this for all types of map sites. But that can be done as well:

class Maze {
public:
    Room& addRoom(int id) {
        sites.push_back(std::make_unique<Room>(id));
        return static_cast<Room&>(*sites.back());
    }

    Door& addDoor(Room& room1, Room& room2) {
        …
    }

    Wall& addWall() {
        …
    }

private:
    std::vector<std::unique_ptr<MapSite>> sites;
};

MazeGame createMaze() {
  Maze maze;
  auto r1 = maze.addRoom(1);
  auto r2 = maze.addRoom(2);
  auto door = maze.addDoor(r1, r2);

  r1.setSide(north, maze.addWall());
  r1.setSide(east, door);
  …
}

Now you can have the site types store raw pointers and/or references:

class Room : public MapSite {
public:
    Room(int id): id(id) {}
    void setSide(Direction direction, MapSite& mapSite) {
        sides[direction] = &mapSite;
    }
    …
private:
    std::array<MapSite*, 4> sides;
    int id;
};

class Door : public MapSite {
public:
    Door(Room& room1, Room& room2): room1(room1), room2(room2) {}
    …
private:
    Room& room1;
    Room& room2;
};

To avoid having to write a new addSomething() function every time you add a new map site type, you can make it a template:

class Maze {
public:
    template<typename T, typename... Args>
    T& add(Args&&... args) {
        sites.push_back(std::make_unique<T>(std::forward<Args>(args)...));
        return static_cast<T&>(*sites.back());
    }

private:
    std::vector<std::unique_ptr<MapSite>> sites;
};

MazeGame createMaze() {
  Maze maze;
  auto r1 = maze.add<Room>(1);
  auto r2 = maze.add<Room>(2);
  auto door = maze.add<Door>(r1, r2);

  r1.setSide(north, maze.add<Wall>());
  r1.setSide(east, door);
  …
}

It's easy to create invalid mazes

What if you do:

r1->setSide(east, door);
…
r2->setSide(south, door);

Now you enter room 2 from the east, but to get back to room 1 you have to go south. What if you forgot to call r2->setSide(…, door) at all? Then you can never leave room 2.

What if you forgot to write:

r1->setSide(north, std::make_shared<Wall>());

Now going north will result in your program crashing.

What if you have three rooms, r1, r2 and r3, and a door between r1 and r2, but then write:

r3->setSide(west, door);

Then that door will be permanently locked. Sure, it's not a crash, but it probably still is a bug.

What if you wrote:

r1->setSide(east, r2);
…
r2->setSide(west, r1);

Looks fine, just a connection between two rooms without a door inbetween. It looks like moving east from room 1 will bring you to room 2, but it only prints "entered room 2", it never updates the player's position.

Room identifiers

You have to manually assign an id to each room. What if you give two rooms the same identifier? Sure, it doesn't cause a crash, but it will confuse the player. I would either:

• Enforce that a unique identifier is provided, perhaps by having a std::unordered_map<int, Room*> to keep track of which room is associated with which identifier.
• Automatically generate a unique identifier when adding a room to the Maze. You could add a counter variable to Maze, or perhaps you can just use the position in which the room is inserted into the vector rooms as the identifier.