Text-based adventure game using classes and XML

Question

I am studying software engineering and we just started learning about object oriented programing. I wanted to be a bit ahead of all others, so I decided to write a little text-adventure using classes.

I would be very glad to know what I can improve and what I can do better. Should I do something differently?

Main.cpp

#include "Game.hpp"

int main(int argc, char const *argv[]) {
  Game game = Game();
  game.play();
  return 0;
}

Game.hpp

#ifndef GAME_HPP
#define GAME_HPP

#include "Menu.hpp"

class Game {
public:
  Game();
  void play();
  void road();
  std::vector<Menu> parseXML(std::string file);
};

#endif

Game.cpp

#include <iostream>
#include <string>
#include <algorithm>
#include <vector>
#include <pugixml.hpp>

#include "Game.hpp"
#include "Menu.hpp"

Game::Game()
{
}

void Game::play()
{
  road();
  std::cout << "...\n";
}

void Game::road()
{
  std::vector<Menu> game = parseXML("game.xml");

  auto menu = std::find(game.begin(), game.end(), "mainroad");
  while (menu != game.end())
  {
    menu = std::find(game.begin(), game.end(), menu->takeChoice());
  }
}

std::vector<Menu> Game::parseXML(std::string file)
{
    std::vector<Menu> game;

    pugi::xml_document doc;
    doc.load_file(file);

    pugi::xml_node menus = doc.child("menus");
    for (pugi::xml_node menu = menus.child("menu"); menu; menu = menu.next_sibling("menu"))
    {
      const pugi::char_t* name = menu.attribute("name").value();
      const pugi::char_t* prompt = menu.child_value("prompt");

      pugi::xml_node choices = menu.child("choices");
      std::vector<std::pair<std::string, std::string>> choices_vector;

      for(pugi::xml_node choice = choices.child("choice"); choice; choice = choice.next_sibling("choice"))
      {
        std::string choice_item = choice.first_child().value();
        std::string choice_link = choice.attribute("link").value();
        choices_vector.push_back({choice_item, choice_link});
      }
      game.push_back(Menu(name, prompt, choices_vector));
    }
    return game;
}

Menu.hpp

#ifndef MENU_HPP
#define MENU_HPP

#include <string>
#include <vector>

class Menu {
public:
  Menu(const std::string&, const std::string&,
      const std::vector<std::pair<std::string, std::string>>&
      = std::vector<std::pair<std::string, std::string>> {});
  const std::string& takeChoice() const;
  bool operator==(const std::string&) const;

private:
  std::string _name, _prompt;
  std::vector<std::pair<std::string, std::string>> _choices;
};

#endif

Menu.cpp

#include <iostream>

#include "Menu.hpp"

Menu::Menu(const std::string &name, const std::string &prompt,
          const std::vector<std::pair<std::string, std::string> > &choices)
: _name(name)
  ,_prompt(prompt)
  , _choices(choices)
{
}

bool Menu::operator==(const std::string &name) const
{
  return name == _name;
}

const std::string& Menu::takeChoice() const
{
  if (_choices.size() == 0)
  {
    std::cout << _prompt;
    return "END";
  }
  unsigned int choice;
  int i;
  do
  {
    std::cout << _prompt;
    i = 1;
    for (auto ch : _choices)
    {
      std::cout << i++ << ": " << ch.first << "\n";
    }
    std::cin >> choice;
    choice--;
  }
  while(choice >= _choices.size());
  return _choices[choice].second;
}

game.xml

<?xml version="1.1" encoding="UTF-8"?>
<menus>
    <menu name="mainroad">
        <prompt>
            You are on a road that heads west east of your position.
            Which way will you go?
        </prompt>
        <choices>
            <choice link="spider">Go West</choice>
            <choice link="brickhouse">Go East</choice>
            <choice link="dragon">Wait for something to happen</choice>
        </choices>
    </menu>

    <menu name="spider">
        <prompt>
            You travel down the road, about only 100 metres and you encounter
            a giant spider with vicious poison coated fangs.
            its hideous appearance causes your throat to dry and your knees to shake!
            What on earth will you do?
        </prompt>
        <choices>
            <choice link="spiderattack">Attempt to attack the spider with your sword.</choice>
            <choice link="throwsword">Throw your sword in the off chance it might kill it.","throwsword</choice>
            <choice link="running">RUN FOR YOUR LIFE!</choice>
        </choices>
    </menu>

    <menu name="spiderattack">
        <prompt>
            You viscously swing your sword at the spiders general direction.
            The swing was so great, your arms jolts out of place,
            creating a surge of pain.
            Your arm is now broken, and you fall to the ground in pain...
            The spider launches 3 metres straight into your body...
            What on earth is it doing?
            Oh My God! The spider is devouring everything...
            All that remained was bones of the once mobile adventurer.
        </prompt>
    </menu>
</menus>

Answer 1

I like when I see a small main() function! Also the way you check the value from std::cin is nice.

Here's what I think could be better:

Reference and copy

You are using a const std::vector<std::pair<std::string, std::string> >& in your Menu constructor. This reference avoids a copy when you call the constructor, but as _choices isn't a reference, you still copy your std::vector during the assignment (and that's fortunate because the original variable is destroyed right after the constructor ends, when the current iteration of the for loop ends in Game::parseXML(std::string file)). To avoid this copy, you could use a move assignment.

std::find and std::vector

I find the way you use std::find a bit confusing. It is easy to understand, but doesn't help readability. Appart from that, you are using an std::vector on which you never iterate but only try to access elements by their key (which is currently an instance, but may be their name according to Menu::operator==). You probably want to use an std::unordered_map instead of an std::vector in Game::road.

Naming

std::vector<Menu> game doesn't look like a good name choice. Maybe something like currentLevel would be better. Same for choice and _choices that could be named playerchoice and availableChoices respectively.

A famous quote from Phil Karlton - There are only two hard things in Computer Science: cache invalidation and naming things. It is important that you take some time to name things correctly.