8
\$\begingroup\$

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>
\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$

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.