HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Text-based adventure game using classes and XML

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
textadventurexmlgameusingbasedclassesand

Problem

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 parseXML(std::string file);
};

#endif


Game.cpp

#include 
#include 
#include 
#include 
#include 

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

Game::Game()
{
}

void Game::play()
{
  road();
  std::cout  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 Game::parseXML(std::string file)
{
    std::vector 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> 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
#include

class Menu {
public:
Menu(const st

Solution

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 >& 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 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.

Context

StackExchange Code Review Q#148599, answer score: 6

Revisions (0)

No revisions yet.