6
\$\begingroup\$

I have written a game. Its a very simple clone of the Dwarf Fortress. The game seems to be working fine at least from my testing. I would like to improve my programming game and fix all the bad design, bad practices and possible performance bottlenecks in my code. So any comment that would help me become a better developer is welcome.

Since the game contains multiple classes it seems kind of pointless to paste every class here so here is the link to repository: https://github.com/th3v0ice/Dwarf_Fortress_Clone

Armor.cpp

#include "Armor.h"

std::vector<std::string> Armor::armor_names = { 
        "Edge of Patience", 
        "Defense of Twilight",
        "Armor of Absorption",
        "Armor of Dominance",
        "Adamantite Cuirass",
        "Golden Armor",
        "Armor of Illusions",
        "Chestplate of Soul",
        "Mail Chestplate",
        "Vengeful Adamantite",
        "Tunic of Fury",
        "Protector of Souls",
        "Chestguard of Time"
    };


std::shared_ptr<Armor> Armor::generateArmor(){
    unsigned seed = time(0);
    srand(seed);
    int def = rand() % 200;
    int idx = rand() % armor_names.size();
    std::shared_ptr<Armor> armor(new Armor(armor_names[idx], def));
    return armor;
}

Armor.h

#pragma once
#include "Item.h"
#include <vector>
#include <memory>

class Armor : public Item
{
private:
    int defense_value;

public:
    static std::vector<std::string> armor_names;

    Armor(std::string _name, int value) : defense_value(value) {
        type = item_type::ITEM_TYPE_ARMOR;
        name = _name;
    };

    int get_defense() { return defense_value; }

    std::string toString(){
        std::string res = "[A] " + name + " (+" + std::to_string(defense_value) + " def)";
        return res;
    }

    static std::shared_ptr<Armor> generateArmor();
    
};

Consumable.cpp

#include "Consumable.h"


std::shared_ptr<Consumable> Consumable::generateConsumable() {
    unsigned seed = time(0);
    srand(seed);
    int hp = rand() % 100;
    std::string hpn = "Lesser Health Potion";
    if(hp > 80)
        hpn = "Greater Health Potion";
    else if(hp > 50)
        hpn = "Great Health Potion";

    std::shared_ptr<Consumable> cons(new Consumable(hpn, hp));

    return cons;
}

Consumable.h

#pragma once
#include "Item.h"
#include <memory>

class Consumable : public Item
{
private:
    std::size_t amount;

public:
    Consumable(std::string _name, std::size_t a) : amount(a) {
        type = item_type::ITEM_TYPE_POTION;
        name = _name;
    };
    ~Consumable() {};

    int get_amount() { return amount; }

    std::string toString(){
        std::string res = "[C] " + name + " (+" + std::to_string(amount) + " health)";
        return res;
    }

    static std::shared_ptr<Consumable> generateConsumable();
};

Entity.cpp

#include "Entity.h"

Entity::Entity()
{
    name = "";
    health = 100;
    armor = nullptr;
    weapon = nullptr;
    attack = 10;
    defense = 0;
    inventory.setLimit(3);
}

bool Entity::reduceHealth(int amount)
{
    health -= amount;
    if (health <= 0)
        return false;

    return true;
}

int Entity::equipOrConsume(std::shared_ptr<Item> i)
{
    switch (i->getType()) {
    case item_type::ITEM_TYPE_ARMOR:
    {
        std::shared_ptr<Armor> a_armor = std::dynamic_pointer_cast<Armor>(i);

        //We first unequip the previous armor if one existed
        if (armor)
            defense -= armor->get_defense();

        defense += a_armor->get_defense();

        armor = a_armor;
        break;
    }
    case item_type::ITEM_TYPE_POTION:
    {
        std::shared_ptr<Consumable> c_cons = std::dynamic_pointer_cast<Consumable>(i);
        health += c_cons->get_amount();
        if (health > 100)
            health = 100;
        break;
    }
    case item_type::ITEM_TYPE_WEAPON:
    {
        std::shared_ptr<Weapon> w_weapon = std::dynamic_pointer_cast<Weapon>(i);

        //Unequip a weapon first
        if (weapon)
            attack -= weapon->get_damage();

        attack += w_weapon->get_damage();

        weapon = w_weapon;
        break;
    }
    default:
    {
        break;
    }
    }

    return 0;
}

Entity.h

#pragma once
#include "Inventory.h"
#include "Weapon.h"
#include "Armor.h"
#include "Consumable.h"
#include "Item.h"
#include <string>
#include <memory>

class Entity
{
protected:
    std::string name;

    std::shared_ptr<Armor> armor;
    std::shared_ptr<Weapon> weapon;

    int 
        health,
        attack,
        defense;


    Inventory inventory;

public:
    Entity();
    ~Entity() {};

    /**
    * Reduces the health of the entity by the given amount.
    * If the health goes to zero or below zero, method will
    * return false, which means that the entity is dead. Otherwise
    * it will return true;
    */
    bool reduceHealth(int amount);

    std::string& getName() {
        return name;
    }

    int getHealth() {
        return health;
    }

    /**
    * Equip will calculate entities armor and attack values based on the
    * equiped item.
    */
    int equipOrConsume(std::shared_ptr<Item> i);

    void testFillInventory(){
        inventory.fillWithDummyData();
    }

    virtual void dropFromInventory() = 0;

    int getAttack() { return attack; }
    int getDefense() { return defense; }


};

Inventory.cpp

#include "Inventory.h"

void Inventory::drawInventory(BUFFER &buffer)
{
    int 
        height = buffer.size(),
        width = buffer[0].size(),
        inv_width =  (width >= 50) ? 50 : width,
        inv_height = (height >= 16) ? 16 : height,
        temp_h_start = height / 2 - inv_height / 2,
        h_start = (temp_h_start > 0) ? temp_h_start : 0,
        temp_w_start = width / 2 - inv_width / 2,
        w_start = (temp_w_start > 0) ? temp_w_start : 0,
        spacing = 2;


    //Clear the buffer in that region
    for (int i = h_start; i < h_start + inv_height; i++) {
        buffer[i][w_start] = '|';
        buffer[i][w_start + inv_width-1] = '|';
        for (int j = w_start + 1; j < w_start + inv_width - 1; j++) {
            buffer[i][j] = ' ';
        }
    }


    //First line ╔════════╗
    for (int j = w_start; j < w_start + inv_width; j++) {
        if(j == w_start)
            buffer[h_start][j] = '+';
        else if(j == w_start + inv_width - 1)
            buffer[h_start][j] = '+';
        else 
            buffer[h_start][j] = '-';
    }

    h_start += spacing;
    
    selected_item_idx = 0;
    selected_item_idx_y = h_start;
    selected_item_idx_x = w_start + spacing;

    //Inventory is limited to 3 items
    for (int i = h_start, inv_cnt = 0; i < h_start + inv_height - spacing, inv_cnt < inventory.size(); i++, inv_cnt++) {
        std::string desc = (inv_cnt == 0) ? "*" : " ";
        desc += inventory[inv_cnt]->toString();

        int start_offset = w_start + spacing;
        for (int j = start_offset, k = 0; j < w_start + inv_width - spacing && k < desc.length(); j++, k++) {
            buffer[i][j] = desc[k];
        }
    }

    if(inv_height < 16)
        h_start = inv_height - 1;
    else
        h_start = h_start + inv_height - 2;

    
    //Last line ╚══Drop(x)══Use(u)═══╝
    std::string usedu = "Drop(x)---Use(u)";
    int cnt = 0;
    for (int j = w_start; j < w_start + inv_width; j++) {
        if(j == w_start)
            buffer[h_start][j] = '+';
        else if(j == w_start + inv_width - 1)
            buffer[h_start][j] = '+';
        else if(j > w_start + spacing && cnt < usedu.length()) {
            buffer[h_start][j] = usedu[cnt];
            cnt++;
        } else 
            buffer[h_start][j] = '-';
    }
    

    return;
}

void Inventory::dropFromInventory()
{
    if(inventory.size() > 0 && inventory.size() > selected_item_idx)
        inventory.erase(inventory.begin() + selected_item_idx);
}

void Inventory::changeInventorySelection(int p, BUFFER &buffer)
{
    buffer[selected_item_idx_y][selected_item_idx_x] = ' ';

    if(inventory.empty())
        return;

    if(p > 0 && selected_item_idx < inventory.size()-1){
        selected_item_idx++;
        selected_item_idx_y++;
    }
    else if(p < 0 && selected_item_idx > 0){
        selected_item_idx--;
        selected_item_idx_y--;
    }

    buffer[selected_item_idx_y][selected_item_idx_x] = '*';

    return;
}

std::shared_ptr<Item> Inventory::getSelectedItem()
{
    if(inventory.size() > 0)
        return inventory[selected_item_idx];

    return std::shared_ptr<Item>(nullptr);
}

int Inventory::addToInventory(std::shared_ptr<Item> item) {

    if(inventory.size() >= limit)
        return -1;

    inventory.push_back(item);
    
    return 0;
}

Inventory.h

#include <vector>
#include <memory>
#include "Item.h"
#include "Weapon.h"
#include "Armor.h"
#include "Consumable.h"
#include "Map.h"

#pragma once
class Inventory
{

public:
    Inventory()     : selected_item_idx(0), limit(3) { inventory.reserve(3);     }
    Inventory(int l): selected_item_idx(0), limit(l) { inventory.reserve(limit); }

    void drawInventory(BUFFER &buffer);
    void dropFromInventory();
    int addToInventory(std::shared_ptr<Item> item);

    void changeInventorySelection(int p, BUFFER &buffer);
    void setLimit(int lim) { limit = lim; }
    int getLimit() { return limit; }
    std::shared_ptr<Item> getSelectedItem();

    void fillWithDummyData(){
        std::shared_ptr<Weapon> w(new Weapon("Mighty sword", 100));
        std::shared_ptr<Armor> a(new Armor("Shiny armor", 20));
        std::shared_ptr<Consumable> c(new Consumable("Small potion", 10));

        std::shared_ptr<Item> w_i = w;
        std::shared_ptr<Item> a_i = a;
        std::shared_ptr<Item> c_i = c;

        inventory.push_back(w_i);
        inventory.push_back(a_i);
        inventory.push_back(c_i);
    }

    std::size_t size() { return inventory.size(); }
    void clear() { inventory.clear(); }
    item_type getItemTypeAtIndex(int i) { return inventory[i]->getType(); };
private:
    std::vector<std::shared_ptr<Item>> inventory;

    int selected_item_idx;
    int limit;
    //Coordinates for a star which designates selected item.
    //It will be much faster to change the selection instead 
    //of drawing everything again.
    int selected_item_idx_x;
    int selected_item_idx_y;
};

Item.h

#pragma once

#include <string>

enum class item_type {
    ITEM_TYPE_ARMOR = 0,
    ITEM_TYPE_WEAPON = 1,
    ITEM_TYPE_POTION = 2
};

class Item
{
private:
    
protected:
    item_type type;
    std::string name;


public:
    Item() {};
    virtual ~Item();

    item_type getType() {
        return type;
    }

    virtual std::string toString() = 0;

    void setType(item_type itype) {
        type = itype;
    }
};

Map.cpp

#include "Map.h"


int Map::loadMap(std::string fileName)
{
    namespace bpt = boost::property_tree;

    std::string data;
    bpt::ptree root;

    try {
        bpt::read_json(fileName, root); //pt::json_parser::json_parser_error
    }
    catch (bpt::json_parser::json_parser_error) {
        std::cout << "Ill formatted JSON file!" << std::endl;
        return BAD_JSON;
    }

    try {
        height = root.get<int>("height");
    }
    catch (bpt::ptree_bad_path) {
        std::cout << "Height is not specified in maps configuration file!" << std::endl;
        return HEIGHT_ERROR;
    }

    try {
        width = root.get<int>("width");
    }
    catch (bpt::ptree_bad_path) {
        std::cout << "Width is not specified in maps configuration file!" << std::endl;
        return WIDTH_ERROR;
    }

    try {
        data = root.get<std::string>("data");
    }
    catch (bpt::ptree_bad_path) {
        std::cout << "Data is not specified in maps configuration file!" << std::endl;
        return DATA_ERROR;
    }

    //Resize the map to accomodate the data and fill the map with blanks
    map.resize((height + FULL_BORDER) * (width + FULL_BORDER), L" ");

    std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> converter;
    std::wstring wide = converter.from_bytes(data);
    
    //Position the data correctly inside the map
    for (int i = 0; i < height; i++) {
        for (int j = 0; j < width; j++) {
            int map_index = (i + BORDER) * (width + FULL_BORDER) + (j + BORDER);
            int data_index = i * width + j;

            map[map_index] = wide[data_index];
        }
    } 

    return 0;
}

int Map::getMapAroundPlayer(int x, int y, int center_x, int center_y, BUFFER& buffer)
{
    //The current position of the player on the screen is (center_x, center_y)
    //The current position of the player on the map is (x, y)

    int 
        map_start_x = x - center_x,
        map_start_y = y - center_y,
        bufh = buffer.size(),
        bufw = buffer[0].size();

    for (int i = 0; i < buffer.size(); i++) {
        for (int j = 0; j < bufw; j++) {
            int map_index = (map_start_y + i) * (width + FULL_BORDER) + map_start_x;
            buffer[i][j] = map[map_index + j];
        }
    }
    
    return 0;
}

int Map::updateMap(int x, int y, data_t c) {

    //The current position of the player on the map is (x, y)
    int
        offset_x = x,
        offset_y = y;

    int map_index = offset_y * (width + FULL_BORDER) + offset_x;
    map[map_index] = c;

    return 0;
}

int Map::self_check()
{
    test_load_bad_file1();
    test_load_bad_file2();
    test_load_bad_file3();
    test_load_bad_file4();


    test_load_good_file();
    test_get_map();
    
    return 0;
}

int Map::test_load_bad_file1()
{
    std::cout << "Invoking test 1 ... " << std::endl;
    int ret = loadMap("tests/bad_test1.json");
    if (ret == HEIGHT_ERROR)
        std::cout << "Pass" << std::endl;
    else
        std::cout << "Fail" << std::endl;

    return 0;
}

int Map::test_load_bad_file2()
{
    std::cout << "Invoking test 2 ... "  << std::endl;
    int ret = loadMap("tests/bad_test2.json");
    if (ret == WIDTH_ERROR)
        std::cout << "Pass" << std::endl;
    else
        std::cout << "Fail" << std::endl;

    return 0;
}

int Map::test_load_bad_file3()
{
    std::cout << "Invoking test 3 ... " << std::endl;
    int ret = loadMap("tests/bad_test3.json");
    if (ret == DATA_ERROR)
        std::cout << "Pass" << std::endl;
    else
        std::cout << "Fail" << std::endl;
    return 0;
}

int Map::test_load_bad_file4()
{
    std::cout << "Invoking test 4 ... " << std::endl;
    int ret = loadMap("tests/bad_test4.json");
    if (ret == BAD_JSON)
        std::cout << "Pass" << std::endl;
    else
        std::cout << "Fail" << std::endl;
    return 0;
}

int Map::test_load_good_file()
{
    std::cout << "Invoking test 5 ... " << std::endl;
    int ret = loadMap("tests/good_test.json");
    if (ret == 0)
        std::cout << "Pass" << std::endl;
    else
        std::cout << "Fail" << std::endl;

    return 0;
}

int Map::draw_map(BUFFER &buffer)
{
    int height = buffer.size();
    int width = buffer[0].size();
    std::wstring buf;
    
    for (int i = 0; i < height; i++) {
        for (int j = 0; j < width; j++) {
            data_t c = buffer[i][j];
            buf.append(c);
        }
    }

    erase();
    move(0,0);
    addwstr(buf.c_str());
    refresh();

    return 0;
}

int Map::test_get_map()
{
    std::chrono::seconds tm(1);
    BUFFER buffer;



    getMapAroundPlayer(92, 15, 185, 30, buffer);
    draw_map(buffer);

    std::this_thread::sleep_for(tm);

    //Move right
    getMapAroundPlayer(93, 15, 185, 30, buffer);
    draw_map(buffer);

    std::this_thread::sleep_for(tm);

    //Move right
    getMapAroundPlayer(94, 15, 185, 30, buffer);
    draw_map(buffer);

    std::this_thread::sleep_for(tm);

    //Move up
    getMapAroundPlayer(94, 14, 185, 30, buffer);
    draw_map(buffer);

    std::this_thread::sleep_for(tm);
    std::this_thread::sleep_for(tm);

    std::this_thread::sleep_for(tm);


    return 0;
}

Map.h

#pragma once
#include <iostream>
#include <vector>
#include <string>
#include <boost/property_tree/ptree.hpp>
#include <boost/property_tree/json_parser.hpp>

#include <locale>
#include <codecvt>
#include <curses.h>


#include <stdio.h>

//Needed for testing.
#include <chrono>
#include <thread>

#include <sstream>

enum {
    HEIGHT_ERROR = -1,
    WIDTH_ERROR = -2,
    DATA_ERROR = -3,
    BAD_JSON = -4
};


/**
* Note: If the console window size changes, we could change the 
* border size accordingly. 
*/
#define FULL_BORDER 400
#define BORDER (FULL_BORDER/2)
#define CLEAR printf("\033[H\033[J")
#define GOTOXY(x,y) printf("\033[%d;%dH", (y), (x))

typedef std::wstring data_t;
typedef std::vector<data_t> VSBUFF;
typedef std::vector<VSBUFF> BUFFER;

class Map
{
private:
    int width;
    int height;



    /**
    * Map is actually bigger by 200 characters on each side.
    * This will enable very fast retrieving of the "view" player
    * is looking at.
    */
    VSBUFF map;

public:
    Map(int w, int h) {
        width = w + FULL_BORDER;
        height = h + FULL_BORDER;
        map.resize(width * height);
    }

    Map() {
        width = 0;
        height = 0;
        map.clear();
    }

    ~Map() {
        map.clear();
    }

    /**
    * Loads the configuration file in JSON format of the map view. Expected
    * value : data pairs are
    * 
    * width   ->   width of the map
    * height  ->   height of the map
    * data    ->   Array of characters representing specific things on the map.
    *              Map is expected to be a single array.
    */
    int loadMap(std::string fileName);


    /**
    * Returns the area around the player given the players coordinates. Player
    * is always centered on the screen. If the player is near a maps bounds 
    * then buffer will be populated with blank's or "space" characters. 
    * 
    * x        ->    is the players X coordinate on the map, not on the screen
    * y        ->    is the players Y coordinate on the map, not on the screen
    * center_x ->    is the center x of the console window
    * center_y ->    is the center y of the console window
    * buffer   ->    is a reference to the vector matrix in which we should
    *                put the screen data
    */
    int getMapAroundPlayer(int x, int y, int bufw, int bufh, BUFFER& buffer);

    /**
     * Updates the map at the designated coordinates. 
     * 
     * x      ->    is the players X coordinate on the map, not on the screen
     * y      ->    is the players Y coordinate on the map, not on the screen
     * c      ->    character to place at coordinates
    */
    int updateMap(int x, int y, data_t c);

    //Testing code
    int self_check();
    int draw_map(BUFFER& buffer);

    int getWidth() { return width; }
    int getHeight() { return height; }

private:
    //UNIT TESTS BELOW

    int test_load_bad_file1();
    int test_load_bad_file2();
    int test_load_bad_file3();
    int test_load_bad_file4();
    
    int test_load_good_file();

    int test_get_map();
};

Monster.cpp


#include "Monster.h"

/**
 * All ASCII Art was downloaded from
 *  
 * https://www.asciiart.eu/mythology
 * 
*/
creature_t Monster::centaur =  {
"  <=======]}======",
"    --.   /|",
"   _\"/_.'/",
" .'._._,.'",
" :/ \\{}/",
"(L  /--',----._",
"    |          \\\\",
"   : /-\\ .'-'\\ / |",
"    \\\\, ||    \\|",
"     \\/ ||    ||"
};

creature_t Monster::ghost = {
"       .-.",
"      ( \" )",
"   /\\_.' '._/\\",
"   |         |",
"    \\       /",
"     \\    /`",
"   (__)  /",
"   `.__.'"
};

creature_t Monster::gryphon = {
"    .-')",
"   (`_^ (    .----`/",
"    ` )  \\_/`   __/     __,",
"    __{   |`  __/      /_/",
"   / _{    \\__/ '--.  //",
"   \\_> \\_\\  >__/    \\((",
"        _/ /` _\\_   |))",
"       /__(  /______/`"
};

std::vector<creature_t> Monster::creatures = {centaur, ghost, gryphon};

void Monster::fillInventoryWithRandomItems() {
    unsigned seed = time(0);
    srand(seed);

    int inv_size = rand() % inventory.getLimit();

    for(int i = 0; i < inv_size; i++) {
        int itm_type = rand() % 3;

        switch(itm_type)
        {
            case 0: {
                std::shared_ptr<Armor> armor = Armor::generateArmor();
                inventory.addToInventory(std::static_pointer_cast<Item>(armor));
                break;
            }
            case 1: {
                std::shared_ptr<Weapon> weapon = Weapon::generateWeapon();
                inventory.addToInventory(std::static_pointer_cast<Item>(weapon));
                break;
            }
            case 2: {
                std::shared_ptr<Consumable> cons = Consumable::generateConsumable();
                inventory.addToInventory(std::static_pointer_cast<Item>(cons));
                break;
            }
        }
    }

    return;
}

std::shared_ptr<Monster> Monster::generateMonster() {
    unsigned seed = time(0);
    srand(seed);

    int att = rand() % 80;
    int def = rand() % 300;

    std::shared_ptr<Monster> monster(new Monster("Monster", att, def));
    monster->fillInventoryWithRandomItems();

    return monster;
}

void Monster::itemsInInventory(int& w, int& a, int& c) {
    w = 0;
    a = 0;
    c = 0;

    for(int i = 0; i < inventory.size(); i++) {
        item_type t = inventory.getItemTypeAtIndex(i);
        switch (t)
        {
        case item_type::ITEM_TYPE_ARMOR:
            a++;
            break;
        case item_type::ITEM_TYPE_WEAPON:
            w++;
            break;
        case item_type::ITEM_TYPE_POTION:
            c++;
            break;    
        default:
            break;
        }
    }

    return;
}

Monster.h

#pragma once
#include "Entity.h"
#include <vector>
#include <memory>
#include <map>

typedef std::vector<std::string> creature_t;

class Monster : public Entity
{
private:
    static creature_t centaur;
    static creature_t ghost;
    static creature_t gryphon;


    static std::vector<creature_t> creatures;

    creature_t shape;

public:
    Monster(std::string _n, int att, int def) {
        name = _n;
        health = 100;
        attack = att;
        defense = def;

        unsigned seed = time(0);
        srand(seed);
        shape = creatures[rand() % 3];
    };
    ~Monster(){};

    void fillInventoryWithRandomItems();

    static std::shared_ptr<Monster> generateMonster();

    void dropFromInventory() { inventory.clear(); };

    void itemsInInventory(int& w, int& a, int& c);

    creature_t getShape() { return shape; }
};

Player.cpp

#include "Player.h"

std::vector<std::string> Player::shape = {
"   \\\\\\|||///",
" .  =======",
"/ \\| O   O |",
"\\ / \\`___'/ ",
" #   _| |_",
"(#) (     )",  
" #\\//|* *|\\\\ ",
" #\\/(  *  )/",   
" #   =====",  
" #   ( U )", 
" #   || ||",
".#---'| |`----.",
"`#----' `-----'"
};

void Player::useSelectedItemFromInventory() {
    std::shared_ptr<Item> item = inventory.getSelectedItem();
    
    if(!item) 
        return;


    inventory.dropFromInventory();
    equipOrConsume(item);

    return;
}

void Player::drawCharacterStats(BUFFER &buffer) {

       int 
        height = buffer.size(),
        width = buffer[0].size(),
        inv_width =  (width >= 50) ? 50 : width,
        inv_height = (height >= 16) ? 16 : height,
        temp_h_start = height / 2 - inv_height / 2,
        h_start = (temp_h_start > 0) ? temp_h_start : 0,
        temp_w_start = width / 2 - inv_width / 2,
        w_start = (temp_w_start > 0) ? temp_w_start : 0,
        spacing = 2;


    //Clear the buffer in that region
    for (int i = h_start; i < h_start + inv_height; i++) {
        buffer[i][w_start] = '|';
        buffer[i][w_start + inv_width-1] = '|';
        for (int j = w_start + 1; j < w_start + inv_width - 1; j++) {
            buffer[i][j] = ' ';
        }
    }


    //First line ╔════════╗
    for (int j = w_start; j < w_start + inv_width; j++) {
        if(j == w_start)
            buffer[h_start][j] = '+';
        else if(j == w_start + inv_width - 1)
            buffer[h_start][j] = '+';
        else 
            buffer[h_start][j] = '-';
    }

    h_start += spacing;
    
    std::string desc;
    int start_offset = w_start + spacing;


#define PRINT_STAT(description, var){\
    std::stringstream ss;\
    ss << description << var;\
    desc = ss.str();\
    h_start++;\
    for (int j = start_offset, k = 0; j < w_start + inv_width - spacing && k < desc.length(); j++, k++)\
            buffer[h_start][j] = desc[k];\
    }
    

    PRINT_STAT("Health:  ", health)
    PRINT_STAT("Attack:  ", attack)
    PRINT_STAT("Defense: ", defense)
    PRINT_STAT("====================" , "-")

    if(armor)
        PRINT_STAT("Armor:   ", armor->toString())
    else
        PRINT_STAT("Armor:   ", "Not equiped")
    
    if(weapon)
        PRINT_STAT("Weapon:  ", weapon->toString())
    else
        PRINT_STAT("Weapon:  ", "Not equiped")

    
#undef PRINT_STAT

    if(inv_height < 16)
        h_start = inv_height - 1;
    else
        h_start = h_start + inv_height - 2;

    
    //Last line ╚══Drop(x)══Use(u)═══╝
    int cnt = 0;
    for (int j = w_start; j < w_start + inv_width; j++) {
        if(j == w_start)
            buffer[h_start][j] = '+';
        else if(j == w_start + inv_width - 1)
            buffer[h_start][j] = '+';
        else 
            buffer[h_start][j] = '-';
    }
    

    return;
}

int Player::addToInventory(std::shared_ptr<Item> item) {
    return inventory.addToInventory(item);
}

Player.h

#pragma once
#include <sstream>
#include "Entity.h"
#include "Map.h"


class Player : public Entity
{
public:
    Player(){
        name = "Player";
        health = 100;
        defense = 10;
        attack = 20;

        inventory.setLimit(5);
    }

    void drawInventory(BUFFER &buffer){
        inventory.drawInventory(buffer);
    }

    void changeInventorySelection(BUFFER &buffer, int p){
        inventory.changeInventorySelection(p, buffer);
        return;
    }

    void dropFromInventory(){
        inventory.dropFromInventory();
    }

    void useSelectedItemFromInventory();
    void drawCharacterStats(BUFFER &buffer);

    int addToInventory(std::shared_ptr<Item> item);

    static std::vector<std::string> shape;
};

View.cpp

#include "View.h"



void View::drawFightScreen(std::shared_ptr<Monster> monster, int& p_hp_x_coord, int& m_hp_x_coord, int& hp_y_coord) {
    //Draw border and fill with blanks
    for (int i = 0; i < height; i++) {
        buffer[i][0] = '|';
        buffer[i][width - 1] = '|';
        for(int j = 1; j < width - 1; j++)
            buffer[i][j] = ' ';
    }
    //Draw border
    for (int j = 0; j < width; j++) {
        buffer[0][j] = '=';
        buffer[height - 1][j] = '=';
    }

    int 
        spacing = 2,
        h_start = 2,
        left_width_start = width - 30;

    //Draw player shape and stats
    for(int i = 0; i < Player::shape.size() && h_start < height-1; i++, h_start++) {
        printMessage(spacing, h_start, Player::shape[i]);
    }
    
    if(h_start + 4 >= height) {
        spacing = 30;
        h_start = 2;
    }

    printMessage(spacing, h_start++, "========================");
    printMessage(spacing, h_start++,   "Attack  " + std::to_string(shrdPlayer->getAttack()));
    printMessage(spacing, h_start++, "Defense " + std::to_string(shrdPlayer->getDefense()));

    p_hp_x_coord = spacing;
    hp_y_coord = h_start;
    printMessage(spacing, h_start++, "Health  " + std::to_string(shrdPlayer->getHealth()));

    creature_t shape = monster->getShape();

    h_start = 2;

    //Draw monster shape and stats
    for(int i = 0; i < shape.size() && h_start < height - 1; i++) {
        printMessage(left_width_start, h_start, shape[i]);
        h_start++;
    }

    if(h_start + 4 >= height) {
        left_width_start -= 30;
        h_start = 2;
    }
    
    printMessage(left_width_start, h_start++, "========================");
    printMessage(left_width_start, h_start++, "Attack  " + std::to_string(monster->getAttack()));
    printMessage(left_width_start, h_start++, "Defense " + std::to_string(monster->getDefense()));

    m_hp_x_coord = left_width_start;
    printMessage(left_width_start, h_start++, "Health  " + std::to_string(monster->getHealth()));
    

    drawMap();

    return;
}

void View::printMessage(int x, int y, std::string s) {
    for(int i = 0; i < s.length() && i < buffer[y].size(); i++)
        buffer[y][x+i] = s[i]; 
    return;
}

int View::initiateFight(std::shared_ptr<Monster> m) {
    //Fight takes turns. Firstly player attacks the monster
    //then the monster fights back. This cycle is repeated
    //until one of the contestans dies.

    bool 
        alive_p = true,
        alive_m = true;
    
    int 
        p_hp_x = 0,
        hp_y = 0,
        m_hp_x = 0;

    drawFightScreen(m, p_hp_x, m_hp_x, hp_y);

    float 
        p_att = shrdPlayer->getAttack(),
        p_def = shrdPlayer->getDefense(),
        m_att = m->getAttack(),
        m_def = m->getDefense();

    nodelay(stdscr, FALSE);

    while (alive_p && alive_m) {
        
        float dmg = p_att * ( p_att / (p_att + m_def));
        if(dmg <= 0) dmg = 1;
        
        alive_m = m->reduceHealth(dmg);

        printMessage(m_hp_x, hp_y, "Health  " + std::to_string(m->getHealth()) + "  ");
        drawMap();
        if(!alive_m)
            break;

        dmg = m_att * ( m_att / ( m_att + p_def));
        if(dmg <= 0) dmg = 1;
        
        alive_p = shrdPlayer->reduceHealth(dmg);
        printMessage(p_hp_x, hp_y, "Health  " + std::to_string(shrdPlayer->getHealth()) + "  ");
        drawMap();


        std::this_thread::sleep_for(std::chrono::milliseconds(1000));
    }

    printMessage(center_x - 12, height - 2, "Press any key to continue");
    drawMap();

    getch();
    nodelay(stdscr, TRUE);

    return (alive_p) ? 0 : 1;
}

int View::checkFieldAndPerfAction(){
    data_t c = buffer[center_y][center_x];
    switch(hash(c))
    {
        case MONSTER: {
            std::shared_ptr<Monster> monster = Monster::generateMonster();
            if(initiateFight(monster)) {
                //Player has died
                printMessage(center_x, center_y, "You have DIED!");
                drawMap();
                std::this_thread::sleep_for(std::chrono::milliseconds(2000));

                return -1;
            } else {
                //std::this_thread::sleep_for(std::chrono::milliseconds(3000));

                shrdMap->updateMap(x_cord, y_cord, L".");
                //We need to drop the items that monster had
                int x = x_cord,
                    y = y_cord, 
                    w, a, c;

                monster->itemsInInventory(w, a, c);
                for(int i = 0; i < w; i++)
                    shrdMap->updateMap(x - i, y, L"W");
                
                y++;
                for(int i = 0; i < a; i++)
                    shrdMap->updateMap(x - i, y, L"A");

                y++;
                for(int i = 0; i < c; i++)
                    shrdMap->updateMap(x - i, y, L"H");    

                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
            }

            break;
        }
        case POTION: {
            std::shared_ptr<Consumable> cons = Consumable::generateConsumable();
            
            if( shrdPlayer->addToInventory(std::static_pointer_cast<Item>(cons)) >= 0)
                shrdMap->updateMap(x_cord, y_cord, L" "); //If Item was consumed we need to update the main map here.

            break;
        }
        case ARMOR: {
            std::shared_ptr<Armor> armor = Armor::generateArmor();

            if( shrdPlayer->addToInventory(std::static_pointer_cast<Item>(armor)) >= 0)
                shrdMap->updateMap(x_cord, y_cord, L" "); //If Item was consumed we need to update the main map here.

            break;
        }
        case WEAPON: {
            std::shared_ptr<Weapon> weapon = Weapon::generateWeapon();

            if( shrdPlayer->addToInventory(std::static_pointer_cast<Item>(weapon)) >= 0)
                shrdMap->updateMap(x_cord, y_cord, L" "); //If Item was consumed we need to update the main map here.         
            break;
        }
        case WALL: {
            y_cord = prev_y;
            x_cord = prev_x;
            shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);

            break;
        }
    }
    return 0;
}

int View::gameLogic(gcode &code) {
    switch(code)
    {
        case gcode::DF_KEY_INVENTORY: {
            if(!inv_open) {
                inv_open = true;
                stats_open = false;
                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                shrdPlayer->drawInventory(buffer);
            } else {
                //Closing the inventory
                inv_open = false;
                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                buffer[center_y][center_x] = 'P';
            }
            break;
        }
        case gcode::DF_KEY_DROP: {
            if(inv_open){
                shrdPlayer->dropFromInventory();
                shrdPlayer->drawInventory(buffer);
            }
            break;
        }
        case gcode::DF_KEY_USE: {
            if(inv_open) {
                shrdPlayer->useSelectedItemFromInventory();
                shrdPlayer->drawInventory(buffer);
            }
            break;
        }
        case gcode::DF_KEY_STATS: {
            if(!stats_open) {
                stats_open = true;
                inv_open = false;
                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                shrdPlayer->drawCharacterStats(buffer);
            } else {
                stats_open = false;
                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                buffer[center_y][center_x] = 'P';
            }
            break;
        }
        case gcode::DF_KEY_UP: {
            if(inv_open && !stats_open){
                shrdPlayer->changeInventorySelection(buffer, -1);
                y_cord = prev_y;        //Forbid moving while inventory is open
            } else if (stats_open) {
                y_cord = prev_y;
            } else {
                y_cord--;
                if (y_cord < BORDER)
                    y_cord = BORDER;

                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                if(checkFieldAndPerfAction() < 0)
                    return -1;

                buffer[center_y][center_x] = 'P';
                prev_y = y_cord;
            }
            break;
        }
        case gcode::DF_KEY_DOWN: {
            if(inv_open && !stats_open){
                shrdPlayer->changeInventorySelection(buffer, 1);
                y_cord = prev_y;        //Forbid moving while inventory is open
            } else if(stats_open) {
                y_cord = prev_y;
            } else {
                y_cord++;
                if(y_cord > limit_y)
                    y_cord = limit_y;

                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                if(checkFieldAndPerfAction() < 0)
                    return -1;

                buffer[center_y][center_x] = 'P';
                prev_y = y_cord;
            }
            break;                
        }
        case gcode::DF_KEY_LEFT: {
            if(inv_open || stats_open){
                x_cord = prev_x;
            } else {
                x_cord--;
                if(x_cord < BORDER)
                    x_cord = BORDER;

                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                if(checkFieldAndPerfAction() < 0)
                    return -1;

                buffer[center_y][center_x] = 'P';
                prev_x = x_cord;
            }
            break;
        }
        case gcode::DF_KEY_RIGHT: {
            if(inv_open || stats_open){
                x_cord = prev_x;
            } else {
                x_cord++;
                if(x_cord > limit_x)
                    x_cord = limit_x;

                shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
                if(checkFieldAndPerfAction() < 0)
                    return -1;

                buffer[center_y][center_x] = 'P';
                prev_x = x_cord;
            }
            break;                
        }
    }

    shrdMap->draw_map(buffer); //Its not neccessary to redraw everytime
    code = gcode::DF_KEY_NONE;

    std::this_thread::sleep_for(std::chrono::milliseconds(30));
    return 0;
}

int View::init() {
    buffer.reserve(height);
    VSBUFF dd(width);

    for(int i = 0; i < height; i++)
        buffer.push_back(dd);

    for(int i = 0; i < height; i++)
        for(int j = 0; j < width; j++)
            buffer[i][j] = ' ';

    std::string buf = "Use WASD or Arrow Keys to move around the map.";
    std::string buf2 = "M represents a Monster and will initiate a fight";
    std::string buf3 = "W is a Weapon pickup";
    std::string buf4 = "A is a Armor pickup";
    std::string buf5 = "H is a Health potion";
    std::string buf6 = "Use I to open the inventory";
    std::string buf7 = "Use C to display player stats";
    std::string buf8 = "Use a move key to start the game!";

    erase();
    move(2, 10); addstr(buf.c_str());
    move(3, 10); addstr(buf2.c_str());
    move(4, 10); addstr(buf3.c_str());
    move(5, 10); addstr(buf4.c_str());
    move(6, 10); addstr(buf5.c_str());
    move(7, 10); addstr(buf6.c_str());
    move(8, 10); addstr(buf7.c_str());
    move(9, 10); addstr(buf8.c_str());
    refresh();

    //drawMap(); 
    shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);


    return 0;
}

int View::drawMap()
{   
    std::wstring buf;
    
    for (int i = 0; i < height; i++) {
        for (int j = 0; j < width; j++) {
            data_t c = buffer[i][j];
            buf.append(c);
        }
    }

    erase();
    move(0,0);
    addwstr(buf.c_str());
    refresh();
    
    return 0;
}

View.h

#pragma once

#include <vector>
#include "Player.h"
#include "Map.h"

#include "Consumable.h"
#include "Armor.h"
#include "Weapon.h"
#include "Monster.h"

#include <curses.h>

enum fields
{
    MONSTER,
    POTION,
    ARMOR,
    WEAPON,
    WALL,
    NONE
};

enum class gcode
{
    DF_KEY_NONE = -1,
    DF_KEY_INVENTORY = 0,
    DF_KEY_MENU = 1,
    DF_KEY_DROP = 2,
    DF_KEY_LEFT = 3,
    DF_KEY_RIGHT = 4,
    DF_KEY_UP = 5,
    DF_KEY_DOWN = 6,
    DF_KEY_USE = 7,
    DF_KEY_STATS = 8
};

class View {
private:
    int 
        width,
        height, 
        x_cord,
        y_cord,
        prev_x,
        prev_y,
        center_x,
        center_y,
        limit_x,
        limit_y;

    bool 
        inv_open,
        stats_open;

    BUFFER buffer;

    std::shared_ptr<Player> shrdPlayer;
    std::shared_ptr<Map> shrdMap;

    int drawMap();
    int checkFieldAndPerfAction();
    int initiateFight(std::shared_ptr<Monster> monster);

    void printMessage(int x, int y, std::string s);

    void drawFightScreen(std::shared_ptr<Monster> monster, int& p_hp_x_coord, int& m_hp_x_coord, int& hp_y_coord);

    fields hash(data_t in) {
        if(in == L"M") return MONSTER;
        if(in == L"W") return WEAPON;
        if(in == L"A") return ARMOR;
        if(in == L"H") return POTION;
        if(in == L"~") return WALL;
        return NONE;
    }

public:
    View(int w, int h, 
        std::shared_ptr<Map> m, 
        std::shared_ptr<Player> p): width(w), height(h), shrdMap(m), shrdPlayer(p) 
    {    
        x_cord = BORDER + m->getWidth() / 2;
        y_cord = BORDER + m->getHeight() / 2;
        prev_x = x_cord;
        prev_y = y_cord;
        center_x = width / 2;
        center_y = height / 2;
        limit_x = BORDER + m->getWidth() - 1;
        limit_y = BORDER + m->getHeight() - 1;

        inv_open = false;
        stats_open = false;
    }
    ~View(){}


    int gameLogic(gcode &code);
    
    int init();

};

Weapon.cpp

#include "Weapon.h"

std::vector<std::string> Weapon::weapon_names = {
        "Tyrhung",
        "Tranquility",
        "King's Legacy",
        "Firestorm Sword",
        "Corrupted Blade",
        "Cataclysm",
        "Worldslayer",
        "Espada",
        "Windsong Protector",
        "Spectral Sword",
        "Armageddon",
        "Severance"
};

std::shared_ptr<Weapon> Weapon::generateWeapon() {
    unsigned seed = time(0);
    srand(seed);
    int att = rand() % 120;
    int idx = rand() % weapon_names.size();
    std::shared_ptr<Weapon> weapon(new Weapon(weapon_names[idx], att));
    return weapon;
}

Weapon.h

#pragma once
#include "Item.h"
#include <vector>
#include <memory>

class Weapon :  public Item
{
private:
    int damage;

public:
    static std::vector<std::string> weapon_names;

    Weapon(std::string _name, int dmg) : damage(dmg) {
        type = item_type::ITEM_TYPE_WEAPON ;
        name = _name;
    }
    
    ~Weapon() {}

    int get_damage() { return damage; }

    std::string toString(){
        std::string res = "[W] " + name + " (+" + std::to_string(damage) + " Att)";
        return res;
    }

    static std::shared_ptr<Weapon> generateWeapon();
};

Can a switch statement be replaced with something more efficient? What are your thoughts about the architecture of the game?

Thanks!

\$\endgroup\$
4
  • \$\begingroup\$ This is a lot of code to peruse: Please provide a "road map" if not a travel guide: Why would someone with an interest in how to code games in C++ well want to read your code? Are there areas that worked out remarkably well? Parts you are not quite comfortable with? (Re) Visit How to get the best value out of Code Review When Asking for ideas. \$\endgroup\$ – greybeard Jan 15 at 12:10
  • \$\begingroup\$ Well I am not sure about how to achieve that. Maybe simple solution would be to read some book about game development. Thanks for reading my post! \$\endgroup\$ – Strahinja Radman Jan 15 at 12:38
  • 3
    \$\begingroup\$ I’m happy to review this, but… just a warning… it’s going to a take a long time because there’s so much code. I prefer to do really detailed reviews, class-by-class, line-by-line, plus an overall design review of patterns and general ideas. If all that sounds good, and you’re willing to wait a week or so, then yup, I’ll be happy to review this. \$\endgroup\$ – indi Jan 15 at 22:49
  • \$\begingroup\$ @indi If it wouldn't be a problem for You because I am ready to learn! \$\endgroup\$ – Strahinja Radman Jan 16 at 5:58
4
\$\begingroup\$

You’ve got some exceptional answers to this question; they’re so good there’s no point to me repeating all the stuff they cover. So I won’t be doing a code review. I’ll just focus on some high-level design review issues.

Avoiding dynamic allocation (especially strings!)

Armor.cpp has this:

std::vector<std::string> Armor::armor_names = { 
        "Edge of Patience", 
        "Defense of Twilight",
        "Armor of Absorption",
        "Armor of Dominance",
        "Adamantite Cuirass",
        "Golden Armor",
        "Armor of Illusions",
        "Chestplate of Soul",
        "Mail Chestplate",
        "Vengeful Adamantite",
        "Tunic of Fury",
        "Protector of Souls",
        "Chestguard of Time"
    };

Now, as G. Sliepen mentioned, this shouldn’t really be a static data member of the Armour class; it should be in an anonymous namespace in Armor.cpp.

But more importantly from a performance perspective, consider what’s really happening here.

  1. All that character data is stored in the executable’s (possibly read-only) data section, and loaded into memory when the program starts.
  2. Space for (at least) 13 strings is dynamically allocated for the vector.
  3. Then 13 individual strings are allocated (assuming no small-string optimization, but those strings look too large for that anyway).
  4. Then the character data is copied from the data section into those 13 strings.

Go back to step 1 there: all the data you actually need is already right there in memory when the program starts. All that dynamic allocation just to eventually copy that data into newly-allocated strings seems… a little wasteful. Couldn’t we just use the character data already there in memory?

Indeed we can, by doing something like this:

// in Armor.cpp:

namespace {

// normally you should never do "using namespace ___;"
//
// but the literals namespaces are specially designed specifically for the
// purpose of being used like that.
using namespace std::string_view_literals;

// in c++20, you could use constinit instead of constexpr
constexpr auto armor_names = std::array{
    "Edge of Patience"sv, 
    "Defense of Twilight"sv,
    "Armor of Absorption"sv,
    // etc....
};

} // anonymous namespace

Instead of constructing strings that dynamically allocate memory and copy constant character data into it, you can use std::string_view, which is only a lightweight reference to character data—internally, it’s basically just a pointer and a size. So armor_names just becomes an array of pointers to already-loaded character data… which means no allocation is done, and no copying.

This idea applies to all data in your program that you want stored within the program’s data; not just string data. If it’s already going to be in memory, there’s no need to dynamically allocate new memory for it and copy it over.

Obviously if you intend to load the armour names from configuration files, or if you intend to do localization, then you do need vectors and strings, but that’s not that big a deal. If you need ’em, you need ’em. This is just about avoiding wasteful allocation or copying that you don’t need.

If you just make this change, nothing about the rest of your code needs to change. (Well, you might need to explicitly convert the armour name to a string in generateArmor(), but we’ll get back to that.) You can make pretty much identical changes in Monster.cpp, Player.cpp, and Weapon.cpp.

Not bad, but can we do any better?

Here’s a design question: Do armours actually need their own, personal copy of the armour name?

This is based on a general design question you should always be asking about your classes. Objects should only be forced to truck around data that is unique to them. For example, imagine you made a Human class… would it make sense for that class to have a int number_of_heads; data member? It’s always going to be 1 (presumably). It’s a waste to have every single instance of a Human class keeping track of how many heads a human has. That should be a static data member, if at all.

That example may sound silly, but this is actually a very deep and subtle idea. Consider a set of animal classes used to keep track of animals in groups:

class animal
{
public:
    virtual ~animal() = default;

    virtual auto name()        const -> std::string = 0;
    virtual auto type_plural() const -> std::string = 0;
    virtual auto grouping()    const -> std::string = 0;
};

class sheep : public animal
{
    std::string _name;
    std::string _type_plural = "sheep";
    std::string _grouping    = "flock";

public:
    explicit sheep(std::string name) : _name(std::move(name)) {}

    auto name() const        -> std::string override { return _name; }
    auto type_plural() const -> std::string override { return _type_plural; }
    auto grouping() const    -> std::string override { return _grouping; }
};

class cow : public animal
{
    std::string _name;
    std::string _type_plural = "cows";
    std::string _grouping    = "herd";

public:
    explicit cow(std::string name) : _name(std::move(name)) {}

    auto name() const        -> std::string override { return _name; }
    auto type_plural() const -> std::string override { return _type_plural; }
    auto grouping() const    -> std::string override { return _grouping; }
};

auto animals = std::vector<std::unique_ptr<animal>>{};

animals.emplace_back(std::unique_ptr{new sheep{"Shaun"}};
// and so on...

for (auto&& p_animal : animals)
    std::cout << p_animal->name() << " is part of a " << p_animal->grouping() << " of " << p_animal->type_plural() << '\n';

Now, the name of each animal is going to be unique to the animal, so each object has to have a string data member to hold that name. BUT! The animal type and the grouping is going to be the same for all animals of the same type. The grouping is going to be "flock" for ALL sheep. So it doesn’t make sense for each sheep object to remember that individually.

Instead you could do this:

class sheep : public animal
{
    static std::string const _type_plural = "sheep";
    static std::string const _grouping    = "flock";

    std::string _name;

public:
    explicit sheep(std::string name) : _name(std::move(name)) {}

    auto name() const        -> std::string override { return _name; }
    auto type_plural() const -> std::string override { return _type_plural; }
    auto grouping() const    -> std::string override { return _grouping; }
};

Now each sheep object is one third the size of before. You’ve also eliminated a whole class of errors and bugs, where somehow the grouping string gets accidentally changed in some member function.

So how does this apply to Armor? Well, consider this design:

// Item.hpp

// Note that Item now has no data members at all. This is how it should be.
// Abstract base classes should generally have no data members. And this
// example actually illustrates why: by letting derived classes decide what
// data members they need, you open up the possibility for optimizations.
class Item
{
public:
    virtual ~Item() = default;

    virtual auto toString() const -> std::string = 0;
};

// Armor.hpp

class Armor : public Item
{
    // If you want to be clever and save even more space, you can decide
    // that the max number of possible armour names is 128 or 255, and use
    // only a single byte to store the index. If you also use a single byte
    // for the DEF, then this whole class will be only 2 bytes large (well,
    // ignoring the vtable pointer).
    std::size_t _name_idx = 0;
    int _def = 0;

    Armor(std::size_t _name, int def) : _name_idx{_name}, _def{def} {}

public:
    static auto generateArmor(RandomEngine&) -> Armor;

    auto toString() const -> std::string override;

    auto get_defense() const noexcept -> int { return _def; }
};

// Armor.cpp

namespace {

using namespace std::string_view_literals;

constexpr auto armor_names = std::array{
    "Edge of Patience"sv, 
    "Defense of Twilight"sv,
    "Armor of Absorption"sv,
    // etc....
};

auto name_dist = std::uniform_int_distribution<std::size_t>{0, armor_names.size() - 1};

auto def_dist = std::uniform_int_distribution{0, 200};

} // anonymous namespace

auto Armor::generateArmor(RandomEngine& rng) -> Armor
{
    auto name = name_dist(rng);
    auto def = def_dist(rng);

    return Armor{name, def};
}

auto Armor::toString() const -> std::string
{
    return "[A] " + std::string{armor_names[_name_idx]} + " (+" + std::to_string(_def) + " def)";

    // OR, a much more efficient implementation:
    using namespace std::string_view_literals;

    constexpr auto s1 = "[A] "sv;
    constexpr auto s2 = " (+"sv;
    constexpr auto s3 = " def)"sv;

    auto buffer = std::array<char, number_of_chars_needed>{};
    auto const [p, _] = std::to_chars(buffer.data(), buffer.data() + buffer.size(), _def);

    auto res = std::string{};
    res.reserve(s1.size() + armor_names[_name_idx].size() + s2.size()
        + (p - buffer.data()) + s3.size());

    res.append(s1);
    res.append(armor_names[_name_idx]);
    res.append(s2);
    res.append(buffer.data(), p);
    res.append(s3);

    return res;
}

This makes the armour class several times smaller (possibly, if you optimize it aggressively enough, just a couple bytes!), and MUCH faster to pass around (because now you’re just copying an index, not a string).

The same idea could be applied all over the code: Consumable, Monster, and Weapon at least.

And once you’ve started down this road, you can start looking for other ways to avoid creating strings—especially temporary strings that just exist to be read and then thrown away immediately. For example, do you really need a toString() function for items at all? The only place I see it used is when rendering inventories or character stats. What if you did something like this instead:

class Item
{
public:
    // for simplicity i'm limiting the description length, but there are ways
    // you could do this where you don't need this limit
    static constexpr auto max_description_length = std::size_t{63};

    virtual ~Item() = default;

    template <std::output_iterator O, std::sentinel_for<O> S>
    auto write_desciption_to(O out, S sen) const -> O
    {
        auto buffer = std::array<char, max_description_length + 1>{};

        auto const end = buffer.data() + buffer.size();
        auto p = do_write_description(buffer.data(), end);

        while (out != sen and p != end)
            *out++ = *p++;

        return out;
    }

protected:
    virtual auto do_write_desciption(char*, char*) const -> char* = 0;
};

class Armor : public Item
{
    // ... [snip] ...

protected:
    // you'd probably actually put this in the cpp file:
    auto do_write_desciption(char* first, char* last) const -> char* override
    {
        using namespace std::string_view_literals;

        constexpr auto s1 = "[A] "sv;
        constexpr auto s2 = " (+"sv;
        constexpr auto s3 = " def)"sv;

        auto buffer = std::array<char, number_of_chars_needed>{};
        auto const [p, _] = std::to_chars(buffer.data(), buffer.data() + buffer.size(), _def);

        auto next = first;
        auto append = [&next, last](auto&& rng)
        {
            auto const range_length = std::ranges::size(rng);
            auto const remaining    = last - next;

            auto const to_copy = std::min<std::size_t>(range_length, remaining);

            if (to_copy)
                next = std::copy_n(std::ranges::begin(rng), to_copy, next);
        };

        append(s1);
        append(armor_names[_name_idx]);
        append(s2);
        append(std::ranges::subrange(buffer.data(), p));
        append(s3);

        return next;
    }
};

With that, in Inventory.cpp, instead of:

for (int i = h_start, inv_cnt = 0; i < h_start + inv_height - spacing, inv_cnt < inventory.size(); i++, inv_cnt++) {
    std::string desc = (inv_cnt == 0) ? "*" : " ";
    desc += inventory[inv_cnt]->toString();

    int start_offset = w_start + spacing;
    for (int j = start_offset, k = 0; j < w_start + inv_width - spacing && k < desc.length(); j++, k++) {
        buffer[i][j] = desc[k];
    }
}

You could do:

// note: bugs left intact; just illustrating how the above is a drop-in replacement
for (int i = h_start, inv_cnt = 0; i < h_start + inv_height - spacing, inv_cnt < inventory.size(); i++, inv_cnt++) {
    int start_offset = w_start + spacing;

    buffer[i][start_offset++] = (inv_cnt == 0) ? "*" : " ";

    auto const first = buffer[i].data() + start_offset;
    auto const last  = first + std::min(buffer[i].size(), /* do that dance to calculate remaining space here */);

    inventory[inv_cnt]->write_desciption_to(first, last);
}

Copying the description directly into the output buffer saves you not one, but two unnecessary strings, with all the allocation and copying that goes along with them. Obviously doing these kinds of optimizations does make the code more complex—it’s so much easier to just copy strings around, right? But if you’re gunning for max speed and min memory usage, this is how you get there. Depending on the cost of allocations, printing the inventory as shown above could be orders of magnitude faster.

Now, let me be clear: this is not about saying “strings are bad”. Strings are somewhat expensive, but… if you need ’em, you need ’em.

The point here is about asking: do you need ’em? When you do, use them. But… if you can avoid them—especially when they’re just throwaway temporaries—then it really is worthwhile to rethink your design and your interfaces to allow avoiding them. And this is a general truth for all expensive or somewhat-expensive types, not just strings.

Avoiding dynamic polymorphism

I know everybody learns in school about object-oriented programming and inheritance and how it’s bee’s knees… but here’s the ugly reality: dynamic polymorphism is the absolute worst design choice you can make in C++. Yeah, I said that.

Allow me to back my position up. There are a number of reasons why you want to avoid designs based on dynamic polymorphism in C++:

  • It requires reference semantics, and C++ is a value-semantic language, so you’re fighting against the language, which is never a good thing.
  • It doesn’t work well with compile-time checking and constexpr, which is bad, because that’s where you get the maximum power of C++.
  • It usually requires dynamic allocation, which is inefficient, cumbersome, and error-prone (though smart pointers do help a lot with the latter).
  • It’s usually much less efficient because of the dynamic allocation, extra vtable pointers, and extra indirection in virtual calls.
  • And there are a ton of nasty gotchas waiting to bite you in the ass, like slicing, if you’re not very careful (particularly with copying/moving/swapping).

Now, don’t get me wrong: I’m not saying that object-oriented programming and dynamic polymorphism are ALWAYS bad. If you’re writing Java, that’s the default solution you should reach for… but C++ ain’t Java. And there are some situations in C++ where dynamic polymorphism is exactly the right solution. (I used it myself in my own game engine, for several things—for example, the game states are all derived from an abstract base class state, so the game’s state machine can switch between arbitrary states (like the world map state, the battle state, the menu state, and so on). This allows for an infinitely extensible amount of game modes, and the cost is negligible, because there’s only a few states active at a time at most, and there are only ~3 virtual calls each game loop.)

I’m not saying never use dynamic polymorphism… I’m just saying to consider every other possible alternative before choosing it.

And in fact, in your design, you’re not really using dynamic polymorphism all that well.

For example, the fundamental reason why you’d create an abstract base class is to allow deriving arbitrary extension classes… including stuff the original base class designer never heard of. Like, when you create a shape base class, you’re leaving the door open for concrete classes for every shape you know (circle, rectangle, triangle)… but also for every shape you’ve never heard of (like astroid, enneadecagon, or tomoe).

It defeats the whole purpose of dynamic polymorphism to have a fixed, closed set of possible derived types. For example, it would be silly to have a base class for the moves in rock-paper-scissors. There are only 3 possible moves: rock, paper, and scissors. You don’t need to allow for infinite possibilities when there are only 3.

Which brings us to this:

enum class item_type {
    ITEM_TYPE_ARMOR = 0,
    ITEM_TYPE_WEAPON = 1,
    ITEM_TYPE_POTION = 2
};

You already know, even before you start defining the Item class, that there are only 3 possibly types of item.

Now, in their answer, Edward suggests using dynamic_cast for this purpose instead… and they’re right, that would be the smarter way to do it. HOWEVER… it’s still bad, and you can see why even in the improved code Edward offers:

int Entity::equipOrConsume(std::unique_ptr<Item> &&i)
{
    auto old{std::move(i)};
    if (Weapon* new_weapon = dynamic_cast<Weapon*>(old.get())) {
        // ... [snip] ...
    }
    else if (Consumable* new_consumable = dynamic_cast<Consumable*>(old.get())) {
        // ... [snip] ...
    }
    else if (Armor* new_armor = dynamic_cast<Armor*>(old.get())) {
        // ... [snip] ...
    }
    return 0;
}

What’s the point of having a polymorphic type if you’re just going to treat every derived type differently anyway?

The problem here is that you have three completely unrelated types—things that really have nothing in common—except that you want to shoehorn them all into an inventory. So you search for a way to link them all together, and all you can come up with is, “well, they’re all ‘things’”. Or, rather, “items”. This isn’t good design; you are not modelling the actual, natural relationship between armour, weapons, and items, you are just creating an artificial connection because you need to store them all in an inventory.

Indeed, look at what the Item base class actually looks like (if we remove the item_type stuff, and data members (abstract base classes should not have data members)):

class Item
{
public:
    virtual ~Item() = default;

    virtual std::string toString() const = 0;
};

The only thing that they all have in common is… you can print ’em. That’s kind of a weak relationship.

So what’s the alternative here?

Well, the most basic alternative is: since you already know what all the possible item types are… just put ’em all in the Item class:

class Item
{
public:
    enum class Type
    {
        armor,
        weapon,
        potion
    };

private:
    Type        _type;
    std::size_t _name_idx;  // used to look up name string in tables
    int         _value;     // has different meaning depending on type

    // private constructor, so you can only create items via the static funcs
    Item(Type, std::size_t, int);

public:
    // randomly generate various item types
    static auto generate_armor(RandomEngine&)  -> Item;
    static auto generate_weapon(RandomEngine&) -> Item;
    static auto generate_potion(RandomEngine&) -> Item;

    auto type() const noexcept -> Type;
    auto value() const noexcept -> int;

    auto to_string() const -> std::string;
};

And you might use it something like this:

int Entity::equipOrConsume(Item const& item)
{
    switch (item.type())
    {
    case Item::Type::armor:
        defence += (armor ? -armor.value() : 0) + item.value();
        armor = item;
        break;
    case Item::Type::potion:
        health = std::clamp(health + item.value(), 0, 100);
        break;
    case Item::Type::weapon:
        attack += (weapon ? -weapon.value() : 0) + item.value();
        weapon = item;
        break;
    }

    return 0;
}

Now, I’m not saying this is a good design. If you add more item types, things will get unwieldy very quickly. But it will be thousands of times more efficient than your current design, what with all the dynamic allocation and indirection and so on.

So what would be a better design? Well, rather than dynamic polymorphism… you could use static polymorphism.

It’ll be much easier to show than explain, but the basic idea is that it doesn’t make sense to shoehorn weapons, armour, and potions into some vague “item” type—they’re obviously completely unrelated things that you just happen to want to stick in the same inventory slots. So let’s make it so:

// for brevity, i'm not going to show the COMPLETE classes for the three
// item types, but you should get the gist of it

class Armor
{
    std::size_t _name_idx;
    int         _def;

public:
    auto def() const noexcept -> int;

    auto to_string() const -> std::string;
};

class Weapon
{
    std::size_t _name_idx;
    int         _atk;

public:
    auto atk() const noexcept -> int;

    auto to_string() const -> std::string;
};

class Potion
{
    std::size_t _name_idx;
    int         _amount;

public:
    auto amount() const noexcept -> int;

    auto to_string() const -> std::string;
};

// and here's where the magic happens...
using Item = std::variant<Armor, Weapon, Potion>;

// or, rather than an ad-hoc alias, you could create an actual item class that
// has the variant as a data member, then rather than the free functions that
// follow, you could make these member functions

// for cases where the behaviour is the same for all item types, you can do
// something like this:
auto to_string(Item const& item)
{
    return std::visit([](auto&& i) { return i.to_string(); }, item);
}

// for cases where the behaviour differs between types, you can do something
// like this:
auto equip_or_consume_item(Entity& entity, Item const& item)
{
    struct
    {
        Entity& entity;
    
        auto operator()(Armor const& armor)
        {
            // adjust the entity's def, and change armour
        }

        auto operator()(Weapon const& weapon)
        {
            // adjust the entity's atk, and change weapon
        }

        auto operator()(Potion const& potion)
        {
            // adjust the entity's health
        }
    } entity_visitor{.entity = entity};

    // if you add a new item type, but forget to add something to handle it
    // in the visitor above, you will get a compile-time error
    //
    // that way you'll never fail to properly update everywhere you need to
    // consider a new item type
    //
    // you could also pull out the struct above, maybe call it select_item_handler,
    // and keep it closer to the entity class - you have design options

    std::visit(entity_visitor, item);
}

As you can see, when the behaviour is the same (such as with to_string()) you can use auto&& as a catch-all default. When you need different behaviour, you can use a more complex visitor, as in equip_or_consume_item() above. Or you can use std::holds_alternative() to detect what type of item it is. Or std::get_if(). You have options. (But using a visitor with the types given, and with no auto&& catch-all, allows you to detect at compile time that you’ve covered all the possible item types.)

This next bit is very advanced, but I want to show you just how powerful and flexible this technique is. You could get really clever, and use concepts to allow for a whole class of “things”, and operate on all them the same way. For example rather than having an armour class, you could have an armour concept, where anything that is armour adds DEF when worn:

// you could obviously make a *much* better concept than this, but i'm just
// illustrating the idea
template <typename T>
concept armour = 
    requires (T t)
    {
        {t.def()} -> std::same_as<int>;
    };

class shield
{
    std::size_t _name_idx;
    int         _def;

public:
    auto def() const noexcept -> int;

    auto to_string() const -> std::string;
};

class helmet
{
    std::size_t _name_idx;
    int         _def;

public:
    auto def() const noexcept -> int;

    auto to_string() const -> std::string;
};

// etc. with more armour types

using item = std::variant<
    shield,
    helmet,
    // other armour types
    sword,
    axe,
    // other weapon types
    minor_potion,
    greater_potion,
    // other potion types
>;

template <entity Entity>
class select_item_handler
{
    Entity& _entity;

    template <armour Armour>
    auto operator()(Armor const& armor)
    {
        // adjust the entity's def, and change armour
        //
        // works with *ANY* entity type, and *ANY* armour type
    }

    // more operators for weapons, potions, and any other types
    //
    // you can special case specific types of armour, weapons, or whatever
    // simply by naming the type explicitly (that is, not using a function
    // template like above)
    //
    // and you can still add a catch-all default with an unconstrained
    // function template
};

That would allow you to have all different kinds of armour, and you could use logic to make sure you only wear one of each type, but they all add to your defence.

But back to the original design, using a variant to hold armour, weapons, and potions allows you to make an inventory without requiring smart pointers:

std::vector<Item> inventory;

That will just work, and it will be much smaller, and much faster to iterate over:

// there are many different ways to iterate over the inventory

// an in-place visitor (good for simple stuff, or where the behaviour is the
// same for all item types):
for (auto&& item : inventory)
    std::cout << std::visit([](auto&& i) { return i.to_string(); }, item) << '\n';

// a separate visitor (for more complex stuff, or where the behaviour has to
// be different for different types):
struct
{
    auto operator()(Potion const& potion)
    {
        using namespace std::string_literals;
        return "a potion"s;
    }

    // fallback function, used for anything that isn't a potion:
    template <typename T>
    auto operator()(T const& t)
    {
        return t.to_string();
    }
} visitor;

for (auto&& item : inventory)
    std::cout << std::visit(visitor, item) << '\n';

// you can use holds_alternative to select specific types:
for (auto&& item : inventory)
{
    // do something with only weapons
    if (std::holds_alternative<Weapon>(item))
        std::cout << "weapon found!\n";
}

// or you can use get_if:
for (auto&& item : inventory)
{
    // do something with only armour
    if (auto const p_armor = std::get_if<Armor>(&item); p_armour != nullptr)
        std::cout << "armour: " << p_armor->to_string() << '\n';
}

ALL of these options are MUCH more efficient than iterating through a vector of smart pointers and either using dynamic_cast or calling virtual functions.

The same way that variant collects all the item types into a compile-time sum type, you could make a variant that collects all the entity types.

This is a completely different way of thinking about program design, and if all you know is classic OO with dynamic polymorphism, it can look weird and wrong. But if your set of types is closed—that is, if you know every type that’s ever going to be used, and you’re not leaving the door open for anything else (which is true for most programs)—then you can get massive performance, usability, and safety benefits from compile-time, static polymorphism. And there are many, many more benefits that I haven’t even been able to cover. Like, for example, with your current, classic-OO, runtime-polymorphic design, if you want to add a new capability to items—like a new function in the item interface, or a whole new interface (like IDrawable, maybe?)—then you will break EVERYTHING; EVERY class that derives from Item or that needs the new interface will have to be adjusted… but with a compile-time polymorphic design like the one illustrated above, it’s trivial to add more functions to the common item interface, or to add whole new interfaces—you can even add these new things bit-by-bit.

Getting smart with randomness

This is only a small additional suggestion. You have already had it suggested by all the other reviewers that you should use the C++ <random> library instead of std::rand(). I third that sentiment; std::rand() is garbage. AND you’re not even using it correctly (not totally your fault, because it is INCREDIBLY hard to use correctly, but doing std::rand() % N to get a random number between 0 and N -1 is wrong).

So okay, I’m going to assume you’re going to use a proper RNG library, and I’m going to assume it’s <random>. If so, then I can give you some advice on how to use it in a game.

First, you usually only need a single random generator for a game. (For something like scientific or engineering simulations, it can be handy to have each “thing” being simulated to have its own RNG, but that’s WAY overkill for a game.) Or, at most, a single RNG per thread. (Having a per-thread RNG means you don’t need to worry about synchronizing your RNG, but it also means your game is now totally unpredictable. You may be thinking: “Good! Unpredictable is good!” No, unpredictable is bad, because it makes testing functionally impossible, and it means you can’t do things replays to debug or detect cheating.)

So assuming you have only a single RNG, you should select a really good one—MT19937 is more than good enough. Then what you want to do is set it up and seed it right at the beginning of the game. The standard setup pattern for <random> engines works fine:

auto prng = std::mt19937{std::random_device{}()};

Probably the best design is to have a class or function that returns a reference to some static RNG object. That way you don’t have to keep passing it around through your entire game code, and if you need to you can mock it out for testing:

// random.hpp

#ifndef THEGAME_RANDOM_HPP
#define THEGAME_RANDOM_HPP

#include <random>

// you should always put your stuff in a namespace
namespace ns {

using random_engine_t = std::mt19937;

auto random_engine() noexcept -> random_engine_t&;

} // namespace ns

#endif // include guard

// random.cpp

#include "random.hpp"

namespace {

auto prng = std::mt19937{std::random_device{}()};

} // anonymous namespace

namespace ns {

auto random_engine() noexcept -> random_engine_t&
{
    return prng;
}

} // namespace ns

Now here’s where the game-specific stuff really comes in. For games, I have found that you only need to think about 4 (well, 3½) distributions:

  • std::uniform_int_distribution
  • std::uniform_real_distribution
  • std::bernoulli_distribution
  • std::normal_distribution

The first two are basically the same—just one for ints and one for doubles. These are the ones you use when you want a number between A and B, and every value between those two numbers is equally likely. For example, rolling a die:

// returns a random value between 1 and 6 inclusive, with each value equally likely
auto roll(random_engine_t& gen)
{
    static auto dist = std::uniform_int_distribution{1, 6};

    return dist(gen);
}

This is basically what you’d use for all your generate functions:

auto Consumable::generateConsumable() -> Consumable
{
    constexpr auto hp_min = 0; // assuming you really want a potion that heals 0 hp
    constexpr auto hp_max = 100;

    constexpr auto hp_greater_min = 80;
    constexpr auto hp_great_min   = 50;

    static auto hp_dist = std::uniform_int_distribution{hp_min, hp_max};

    auto const hp = hp_dist(random_engine());

    auto const name = [&] {
        if (hp > hp_greater_min)
            return name_greater_health_potion;
        if (hp > hp_great_min)
            return name_great_health_potion;
        return name_health_potion;
    }();

    return Consumable{name, hp};
}

std::uniform_real_distribution is the same idea, but for cases when you want a floating-point number.

std::bernoulli_distribution is what you’d use for win/lose or pass/fail situations. For example, if there’s a spell that will insta-kill 5% of the time, you could do:

constexpr auto death_spell_success_rate = 0.05;

auto death_spell_success_dist = std::bernoulli_distribution{death_spell_success_rate};

// when casting the spell
if (death_spell_success_dist(random_engine()))
    std::cout << "insta-kill";
else
    std::cout << "spell has failed";

std::normal_distribution is my favourite distribution for adding just a little bit of spice to a game. Let’s say you have a 150 ATK sword of smiting, and your enemy is wearing +50 DEF leather underwear… you do your damage calculation according to your game’s rules, and discover the effective damage from the attack is 100 HP. You’ve done your check for whether the attack is a miss or blocked or whatever (perhaps using std::bernoulli_distribution), and now it’s time to figure out the actual damage done from the attack.

Now you could just say: “well, the damage is 100 HP,” and be done with it. But… that makes for a pretty static and uninteresting game. A player can simply sit there with a calculator and predict the outcome of a battle. Yawn.

Instead what you could do is add a little bit of randomness to the damage. But, here’s the thing! You don’t want to add too much randomness! If the nominal damage from an attack is 100 HP, you don’t want to shock the player by saying it actually only did 2 HP of damage. Or 200 HP. That would just annoy the player.

You want the damage to be roughly 100 HP. Like, 101 HP is fine. So is 99 HP. 105 HP is also ok. 70 HP is… unlikely. 180 HP is very unlikely. This is exactly what a normal distribution is for:

auto nominal_damage = 100;
auto stddev  = nominal_damage * 0.05; // ~70% of attacks will be with +/-5% of the nominal

auto damage_dist = std::normal_distribution{nomimal_damage, stddev};

auto const damage_d = damage_dist(random_engine());
auto const damage = static_cast<int>(damage_d); // because you probably want the result as an int

Normal distributions make a game much more interesting. In the example above, I set the standard deviation at 5%… but you could even vary that. For example, you could have a berserker mode where the deviation is more like 30%: that means you could hit a lot harder… but you could also swing wild and hit a lot weaker.

Summary

I’ve tried to give a very high-level overview of design decisions in your code; for actual code reviews, I think the other answers you already have are excellent.

The main problems with your existing design are:

  • WAAAAY too much dynamic allocation. A lot of that is a consequence of using dynamic polymorphism.
  • Too much unnecessary copying of data. If you modify your interfaces to access data directly, or to copy data directly into output buffers, that will speed things up considerably.

You should also consider moving away from a design based on dynamic polymorphism, and look into static (compile-time) polymorphism. That requires a MASSIVE re-imagining of your design and code, but it does also come with massive performance gains.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for this detailed answer. I will mark it as accepted because it touches the architecture of the code and addresses the things I could do differently, which is what I was looking for. \$\endgroup\$ – Strahinja Radman Jan 20 at 7:16
6
\$\begingroup\$

Here are a number of things to help you improve your code.

Write member initializers in declaration order

The View class has this constructor:

View(int w, int h, 
    std::shared_ptr<Map> m, 
    std::shared_ptr<Player> p): width(w), height(h), shrdMap(m), shrdPlayer(p) 
{    /* ... */ }

That looks fine, but in fact, shrdPlayer will be initialized before shrdMap because members are always initialized in declaration order and shrdPlayer is declared before shrdMap in this class. To avoid misleading another programmer, you should swap the order of declaration of those two class members.

Be careful with signed and unsigned

In many cases, the code compares an int i with an unsigned size_t size. It would be better to declare i to also be size_t.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef VIEW_H
#define VIEW_H
// file contents go here
#endif // VIEW_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Don't define an empty destructor

The current ~View is empty. Better would be to simply omit it. See C.30.

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. For instance, although type is a protected data member of Item, anything is allowed to either setType() or getType() rendering any protection meaningless. Better would be to simply declare it public or to eliminate setType() which is never used. If you do keep getType, make it a const function. See C.131

Don't reseed the random number generator more than once

The program currently calls srand in many places in the code. This is really neither necessary nor advisable. Instead, just call it once when the program begins and then continue to use rand() to get random numbers. Better yet, see the next suggestion.

Consider using a better random number generator

Since you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_int_distribution and friends in the <random> header.

Use const where practical

Many of the small helper functions return a value but don't alter the underlying object. These should be const:

int getAttack() const { return attack; }
int getDefense() const { return defense; }

Separate test code

There are a number of things such as testFillInventory which are clearly intended solely for testing. It's a great idea to write tests, but the better strategy is to separate the test code from the main code.

Don't define a default constructor that only initializes data members

Instead of this:

Entity::Entity()
{
    name = "";
    health = 100;
    armor = nullptr;
    weapon = nullptr;
    attack = 10;
    defense = 0;
    inventory.setLimit(3);
}

Better would be to use in-class member initializers. See C.45

Provide rational constructors for base classes

In a similar vein to the above, the Player constructor is currently this:

Player(){
    name = "Player";
    health = 100;
    defense = 10;
    attack = 20;

    inventory{5};
}

Instead, I'd suggest creating a constructor like this:

Entity::Entity(std::string name, int defense, int attack, int inv_limit)
    : name{name}
    , defense{defense}
    , attack{attack}
    , inventory{inv_limit}
{}

Now the Player constructor could be this:

Player()
    : Entity{"Player", 10, 20, 5}
{}

And the Monster constructor is this:

Monster(std::string _n, int att, int def) 
    : Entity{_n, def, att, 3}
    , shape{creatures[rand() % 3]}
{}

Reconsider the use of pointers

The use of std::shared_ptr<> is better than using raw pointers, but in this case, it seems more likely that std::unique_ptr<> is a better choice. There is only one owner for a Weapon or Armor object and when the player picks one up, it transfers ownership of that object. For that reason, I'd suggest rewriting addToInventory like this:

    int Inventory::addToInventory(std::unique_ptr<Item> &&item) {
        if(inventory.size() >= limit)
            return -1;
        inventory.push_back(std::move(item));
        return 0;
    }

Note that we're using move semantics because we're not duplicating the item but simply moving it to a new owner.

Don't reinvent polymorphism

Whenever you find yourself writing code like this:

    int Entity::equipOrConsume(std::shared_ptr<Item> i)
    {
        switch (i->getType()) {
        case item_type::ITEM_TYPE_ARMOR:
        {
            std::shared_ptr<Armor> a_armor = std::dynamic_pointer_cast<Armor>(i);

stop and reconsider your design. We want to do a different thing if we're passed Armor or Consumable. A better way to do that is to use dynamic_cast which will check at runtime whether casting to a derived object is possible. So for example,

    int Entity::equipOrConsume(std::unique_ptr<Item> &&i)
    {
        auto old{std::move(i)};
        if (Weapon* new_weapon = dynamic_cast<Weapon*>(old.get())) {
            old.release();
            if (weapon) {
                attack -= weapon->get_damage();
            }
            weapon.reset(new_weapon);
            std::cerr << "Using weapon " << weapon->toString() << '\n';
            attack += weapon->get_damage();
        }
        else if (Consumable* new_consumable = dynamic_cast<Consumable*>(old.get())) {
            health += new_consumable->get_amount();
            if (health > 100)
                health = 100;
            std::cerr << "Using potion " << new_consumable->toString() << '\n';
            old.reset();
        }
        else if (Armor* new_armor = dynamic_cast<Armor*>(old.get())) {
            old.release();
            if (armor)
                defense -= armor->get_defense();
            armor.reset(new_armor);
            std::cerr << "Using armor " << armor->toString() << '\n';
            defense += armor->get_defense();
        }
        return 0;
    }

Reconsider naming of enum class items

Because the enum class requires naming the class for each use, I would suggest that rather than item_type::ITEM_TYPE_ARMOR it is just as descriptive to write item_type::ARMOR.

Eliminate spurious semicolons

In lines like this within Entity.cpp

    ~Consumable() {};

the trailing semicolon is not needed.

Reconsider class members

Instead of item_type being a field, it seems to me that it would make more sense for the getType() class to be pure virtual in the base class and then have an override for each derived type, like this:

    item_type getType() const override { return item_type::POTION; }

Know your library

There are a number of lines that look like this:

    std::string buf3 = "W is a Weapon pickup";
    std::string buf4 = "A is a Armor pickup";
    // etc.
    erase();
    move(4, 10); addstr(buf3.c_str());
    move(5, 10); addstr(buf4.c_str());

That's not at all necessary. Instead use mvaddstr like this:

    mvaddstr(4, 10, "W is a Weapon pickup");
    mvaddstr(5, 10, "A is a Armor pickup");

Use appropriate data types

At the moment, the code has these definitions:

typedef std::wstring data_t;
typedef std::vector<data_t> VSBUFF;
typedef std::vector<VSBUFF> BUFFER;

This is almost certainly not what you really want. The smallest unit here should be a wchar_t, not a std::wstring. If you instead define these like so:

typedef wchar_t data_t;
typedef std::wstring VSBUFF;
typedef std::vector<VSBUFF> BUFFER;

You will find that a great many things become much easier. For example, the View::init, using the suggestion above as well, becomes as simple as this:

void View::init() {
    buffer.reserve(height);
    VSBUFF dd(width, ' ');
    for(int i = 0; i < height; i++)
        buffer.push_back(VSBUFF{width, ' '});

    erase();
    mvaddstr(2, 10, "Use WASD or Arrow Keys to move around the map.");
    mvaddstr(3, 10, "M represents a Monster and will initiate a fight");
    mvaddstr(4, 10, "W is a Weapon pickup");
    mvaddstr(5, 10, "A is a Armor pickup");
    mvaddstr(6, 10, "H is a Health potion");
    mvaddstr(7, 10, "Use I to open the inventory");
    mvaddstr(8, 10, "Use C to display player stats");
    mvaddstr(9, 10, "Use a move key to start the game!");
    refresh();

    shrdMap->getMapAroundPlayer(x_cord, y_cord, center_x, center_y, buffer);
}

Clean up on exit

The curses library requires you to call endwin when the program ends. The simplest way to do that is to use the atexit() function:

    void finish() {
        endwin();
    }

Then within main:

    initscr();
    atexit(finish);

Catch errors by reference

The code includes a few lines like this:

catch (bpt::json_parser::json_parser_error) {

This has the effect of catching the error by value and then throwing it away.

Avoid using function-like macros

The PRINT_STAT macro is not a good idea. It's prone to error and is not very efficient. I would suggest that if the entire function is rewritten, the need for it goes away anyway. See ES.31 for details.

Think of the user

Other than dying, there's no way for the user to end the program gracefully. Think of adding an exit key sequence. Also, adding color would greatly enhance the play of the game.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank You very much!!! Great insights! One question, is this line really needed: auto old{std::move(i)};? \$\endgroup\$ – Strahinja Radman Jan 16 at 5:50
  • 1
    \$\begingroup\$ No, that line isn't needed. One can simply use i where old is referenced in that function. \$\endgroup\$ – Edward Jan 16 at 6:36
4
\$\begingroup\$

Terminology

I would not call your game a clone of Dwarf Fortress, for that it is way too simple, and the only similarity I see is that you have an overworld map rendered using ASCII characters. It would be more honest to just call it a roguelike, unless of course you really plan to turn this into a highly detailed world simulation with a colony of dwarves living in it.

You have a class Entity, which can be either a Player or a Monster. However, the word "entity" is often used in game engines to mean any kind of object, or even non-objects like light or sound sources. It might be better to rename your Entity to Character, as you have "player characters" (you) and "non-player characters" (monsters and allies).

Be consistent when naming things

Choose whether to use snake_case or camelCase for function names, and stick to it. I see both ways used, for example Armor::get_defense() vs Entity::getHealth().

Also avoid very short names like m. Is it a Monster or a Map? Only use single-character variable names for things like loop counters (i, j), coordinates (x, y, z) and perhaps width/heights (w, h).

Prefer using std::make_shared<>()

Avoid calling new manually. If you need to create a std::shared_ptr of some object, use std::make_shared<>() to create it, like so:

std::shared_ptr<Armor> Armor::generateArmor() {
    ...
    return std::make_shared<Armor>(armor_names[idx], def);
}

Use proper random number generators

Don't use srand() and rand(), those are old C functions that are quite bad random number generators. Furthermore, using the modulo operator will not give you correctly distributed random numbers. The proper way to generate random numbers in C++ is to use define a random engine that provides randomness, seed it using a std::random_device, and then use a class like std::uniform_int_distribution to generate properly distributed numbers within a given range. Make sure the random engine is declared only once as it is rather expensive to construct, but it is normally fine to create a throw-away distribution. So:

static std::random_device random_device;
static std::default_random_engine random_engine(random_device());

std::shared_ptr<Armor> Armor::generateArmor() {
    std::uniform_int_distribution def_distrib(0, 200);
    std::uniform_int_distribution idx_distrib(0, armor_names.size());
    int def = def_distrib(random_engine);
    int idx = idx_distrib(random_engine);
    return std::make_shared<Armor>(armor_names[idx], def);
}

Move armor_names out of class Armor

The vector of strings armor_names doesn't look like it should be accessible by other objects, it's just a list of names used by generateArmor() internally. So you could make it private, but I would just remove it completely from class Armor, and instead just declare it only in Armor.cpp, like so:

static std::vector<std::string> armor_names = {
    ...
};

Avoid writing empty constructors and destructors

I see various empty destructors, like:

class Consumable: public Item {
    ...
    ~Consumable() {};
    ...
};

But even empty constructors:

class Item {
    ...
    Item() {};
    ...
};

This is not useful, and you should remove them. Sometimes you have multiple overloads for the constructor, and you want to keep the one without parameters, in that case you can tell the compiler to bring back the default constructor like so:

    Item() = default;

Preferable way to initialize member variables

Initializing member variables inside the body of the constructor is something that you should avoid. It can be less efficient than using a member initializer list or default member initializers, and it might also be problematic if exceptions can be thrown while constructing an object. So prefer default member initializers (to avoid having to repeat yourself when you have multiple constructors for a class), then member initializer lists, and only initialize in the body if the other two methods are not possible.

If you have derived classes that need to initialize members of the base class, the best way would be to provide a constructor for the base class that allows that to be done. For example:

class Entity {
    std::string name;

    std::shared_ptr<Armor> armor;
    std::shared_ptr<Weapon> weapon;

    int health{100};
    int attack{10};
    int defense{0};

    Inventory inventory{3};

public:
    Entity() = default;
    Entity(const std::string &name, ...): name(name), ... {}
    ...
};

class Player: public Entity {
    Player(): Entity("Player", ...) {}
    ...
};

Avoid unnecessary std::shared_ptrs

You make everything a std::shared_ptr, but this is often not necessary. Shared pointers should be used when ownership of an object has to be shared, but if there is a unique owner then you don't need it, and a std::shared_ptr would then only add unnecessary overhead.

For example, you only generate a Monster in checkFieldAndPerfAction(), and after the fight the monster is gone. So you could have just declared monster as a regular variable:

case MONSTER: {
    Monster monster = ...;
    if (initiateFight(monster)) {
        ...

Of course this will require other changes: Monster::generateMonster() should return a Monster by value, View::initiateFight() should take a reference to a Monster, and so on.

It also looks like class View "owns" most things, like the player and the map, so those don't have to be held by std::shared_ptrs either. It's likely that you don't need any std::shared_ptrs at all.

Avoid macros

In C++ you almost never need to use macros. For example, PRINT_STAT() can be replaced by a lambda function:

auto print_stat = [](const auto &description, const auto &var){
    std::stringstream ss;
    ss << description << var;
    ...
};

print_stat("Health:  ", health);
...

Use raw string literals for the ASCII art

C++11 introduced raw string literals that make it much nicer to write (long) strings that contain characters that you would normally have to escape. For example:

creature_t Monster::centaur =  {
R"(  <=======]}======)",
R"(    --.   /|      )",
R"(   _"/_.'/        )",
R"( .'._._,.'        )",
R"( :/ \{}/          )",
R"((L  /--',----._   )",
R"(    |          \\ )",
R"(   : /-\ .'-'\ / |)",
R"(    \\, ||    \|  )",
R"(     \/ ||    ||  )"
};
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for great insights! I wasn't aware of the raw string literals and I tend to mix snake_case and camelCase because i work with some libraries that use both. \$\endgroup\$ – Strahinja Radman Jan 16 at 5:57

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.