6
\$\begingroup\$

I'm working on writing a text based RPG game, and I've decided to define each possible item as a JSON file in a specific directory. The item factory is meant to act as a singleton, and uses static members to store all possible items. I feel like the whole structure might need a redesign, because I am told that singletons are evil. Also feel free to point anything out that I might be doing that will come back to bite me.

ItemFactory.hpp

#ifndef ITEMFACTORY__HPP
#define ITEMFACTORY__HPP

#include "Item.hpp"

/**
 * \brief Generates new item objects from the defaults that are available.
 *        
 *        Allows for the location files to define items by a code or string
 *        rather than listing all of their attributes. Also better validation
 *        is added by restricting the items to being defined in a directory.
 */
class ItemFactory {
    // Attributes
    private :
        static std::vector<Item> m_validItems;
        static std::map<std::string,std::vector<std::string>> m_validTypes;

        void loadItem(std::string file);

        void loadValidItemTypes(std::string itemTypesFile);
    // Operations
    public :
        ItemFactory (std::string itemDir);
        ItemFactory ();

        /**
         * \brief decides whether the type listed is valid.
         * \param name (???)
         * \return bool
         */
        bool validateType (std::string type);

        /**\return copy of item requested, null if not found.
         */
        Item* getByName (std::string name);

        void listItems (std::ostream& out);
};

#endif

ItemFactory.cpp

#include "../include/ItemFactory.hpp"
#include "../lib/tinydir.h"
#include "../include/Misc.h"
#include <algorithm>
#include <fstream>

std::map<std::string,std::vector<std::string>> ItemFactory::m_validTypes = {};
std::vector<Item> ItemFactory::m_validItems = {};


ItemFactory::ItemFactory (std::string itemDir) {
    this->loadValidItemTypes("config/itemtypes.json");

    // load valid items
    tinydir_dir dir;
    tinydir_open(&dir, itemDir.c_str());
    while(dir.has_next) {
        tinydir_file file;
        tinydir_readfile(&dir, &file);
        if(file.is_reg) {
            this->loadItem(file.path);
        }
        tinydir_next(&dir);
    }
    tinydir_close(&dir);
}

ItemFactory::ItemFactory() { }

void ItemFactory::loadItem(std::string file) {
    json j;
    JSON_FROM_FILE(j, file);

    Item itm(j, this);
    m_validItems.push_back(itm);
}

bool ItemFactory::validateType(std::string type) {
    return m_validTypes.count(type);
}

void ItemFactory::listItems(std::ostream& out) {
    for(auto& itm : m_validItems)
        out << itm.getName() << " - " << itm.getType() << std::endl;
}

void ItemFactory::loadValidItemTypes(std::string itemTypesFile) {
    json j;
    JSON_FROM_FILE(j, itemTypesFile);
    if(!j.is_object())
        throw "ItemTypesFile must contain a json object";
    for(json::iterator it = j.begin(); it != j.end(); ++it) {
        std::vector<std::string> v;
        //TODO: add loading of valid attributes for each type
        m_validTypes.insert(std::make_pair((std::string)it.key(), v));
    }
}

Item* ItemFactory::getByName(std::string name) {
    auto it = std::find_if(m_validItems.begin(), m_validItems.end(),
        [&name](Item const& item) {
            return item.getName() == name;
        });
    if(it == m_validItems.end())
        throw "Can't find requested item";
    return new Item(*it);
}

Item.hpp

#ifndef ITEM__HPP
#define ITEM__HPP

#include "EntityBase.hpp"
#include <map>

class ItemFactory;

class Item : public EntityBase {
    // Attributes
    private :
        std::string m_type;
        std::map<std::string,int> m_attributes;
        /// Price to buy from a store
        int m_buyPrice;
        /// Money returned for selling to store.
        int m_sellPrice;

        ItemFactory* m_factory;
    public:
        Item(json j, ItemFactory* factory);
        Item(const Item& itm);

        std::string getType();
};

#include "ItemFactory.hpp"

#endif
\$\endgroup\$
2
\$\begingroup\$

General:

  • Pass std::string by const&, so void loadItem(std::string const& file); etc.
  • I don't know what the json type looks like, but it should probably be passed by const& into the Item constructor too.
  • The // Attributes and // Operations comments aren't really true (and they're rather unnecessary).
  • There's no reason to #include "ItemFactory.hpp" at the end of Item.hpp. This should be done at the top of the cpp file instead.

Const Correctness:

  • Any function that doesn't modify the state of the class instance (i.e. member variables) should be declared const (e.g. bool validateType(std::string const& type) const;).
  • Any function that doesn't need access to members of a class instance should be static. (In this case, that could actually be all of the functions!)
  • It looks like all the public ItemFactory member functions should be const or static, so Item should probably store a const* to the ItemFactory.
  • In listItems(), the for loop should use auto const&, as the items aren't being altered.

Design:

It's true that singletons have downsides, and should generally be avoided. But this class has problems with consistency, and isn't a true singleton either.

The class should either be completely static (so that there's no reason to create an instance of it):

class ItemFactory {
    private :
        static std::vector<Item> m_validItems;
        static std::map<std::string,std::vector<std::string>> m_validTypes;

        static void loadItem(std::string const& file);
        static void loadValidItemTypes(std::string const& itemTypesFile);

    public :

        static void loadItems(std::string const& itemDir); // constructor replaced with this function!

        static bool validateType(std::string const& type);
        static Item* getByName (std::string name);
        static void listItems (std::ostream& out);
};

...

void doSomething() {
    ItemFactory::validateType("foo"); // don't need an instance to use the factory
}

Or (better) the class should have no static members (and no default constructor) and be passed by const& to where it's needed:

ItemFactory factory("itemDir");

doSomethingWithItemFactory(factory);

...

void doSomethingWithItemFactory(ItemFactory const& factory) {
    factory.validateType("foo"); // use the factory
}

This ensures that the factory is only used after loading the items.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.