c++ class design

[xenovia12]

hello, im writing a game, but im not sure if my classes has a good design

would this be a good design for my class?

class Map
{
    Map(filename) : mapname(filename) {}
    void load()
    {
       holder = MapManager::getMapManagerInstance().loadMap(filename);
    }
    void unload()
    {
        holder.clear(); //unload when not active so save memory
    }
    bool isLoaded();
   
    std::string getmapname();
    std::vector<vector2> holder;
    std::string mapname;
}

class MapManager
{
public:
    static MapManager& getMapManagerInstance(); // returns static object of MapManager
    std::vector<vector2> loadMap(filename)
    {
         if(isMapFileValid(filename))
         //loads map
    }
    
    bool isMapFileValid(filename);
private:
    std::ifstream fopener;
}

int main()
{
      
 std::vector<Map> maps; 
  

   maps.emplace_back("map1.txt");
   maps.emplace_back("map2.txt");
   maps.emplace_back("map3.txt");

   //list the map names 

   //user picks map 1
   maps[0].load();
   
   //user changes his mind and want to pick map 2
   map[0].unload();
   map[1].load();
   
}

and also im thinking another design like having only class MapManager and remove class Map
then it would be like this:

class MapManager
{
public:
    static MapManager& getMapManagerInstance(); // returns static object of MapManager
    void loadMap(filename)
    {
          unloadMap(); unloads first
          if(isMapFileValid(filename))
         //loads map to currentmap vector
    }
    
    void unloadMap()
    {
        //unload map when the game ends or he goes to main menu
    }
    std::vector<std::string> getmapnames();

    bool isMapFileValid(filename);

    void addMap(filename) 
    {
        mapnames.emplace_back(filename);
    }
private:
    std::ifstream fopener;
    std::vector<std::string> mapnames;
    std::vector<vector2> currentmap;
    
}

int main()
{
 MapManager::getMapManagerInstance().addMap("map1.txt");
 MapManager::getMapManagerInstance().addMap("map2.txt");
 MapManager::getMapManagerInstance().addMap("map3.txt");


std::vector<std::string> names = MapManager::getMapManagerInstance().getmapnames();

// list the map names
 
std::string pickedmap;

   //user picks map 1
   MapManager::getMapManagerInstance().load(pickedmap);
   
   //user changes his mind and want to pick map 2
   MapManager::getMapManagerInstance().load(pickedmap);

}

what do you guys prefer here? or if both design sucks then what should i do?

[doug4]

I like the idea of having a Map as a separate class from the Manager. That way, you encapsulate operations on the map separately from managing the collection of maps.

In your first example, however, I would put the std::vector inside the Manager. Since the Manager is ostensibly managing the maps, then it should own the vector that contains them.

There are a whole lot of things I don't know (like what a vector2 is, why a std::vector<vector2> needs to be returned to the used when a map is loaded [rather than a Map reference perhaps], etc.), so I can't comment on whether the overall design makes sense, but separating map functions from the management functions is probably a good idea.

[xenovia12]

std::vector<vector2> holder;
this vector holds the map to render on the screen

 maps[0].load();

so after this call , the holder now holds all the content of filename to be rendered on the screen

so if i put it in the MapManager class i'd have to do this

std::vector<std::vector<vector2>> holder;

should i still move it there?

[doug4]

should I still move it there?

The better question might be "what are the purpose and responsibility of the Manager class?" When I glanced through your code, my assumption was that the class was responsible for managing a set of Map objects and presenting the correct object when requested. That may not be what you intended. You may have wanted the Map Manager to simply load a Map when needed, in which case, the class name may be a bit misleading IMO.

When you properly define the scope of the class, what objects it contains will become obvious. Careful naming of the classes will help, especially when others are invited into the design/review process.

So, to answer your question, it depends. What are you trying to do with it?

[Cubbi]

A good guideline for making something a class is whether there is an invariant over its data members: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-struct

class Map passes that (although the explicit delayed initailization is questionable), but there is no meaning to "MapManager". What is its state? What does its constructor do? To make matters worse, it's a singleton, almost certainly a design mistake: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-singleton

if it were a set of map objects with some sort of selector, as doug4 expected, it could make sense, as long as it's not a singleton. If it's just a collection of maps, there's std::vector for that.

[xenovia12]

@doug4

in the first design:
i made my MapManager to be like a file reader and checker. because the matrix of map is stored in a txtfile, and the loader of the txtfile's content is MapManager's role.


in the 2nd design:
still the same role but in addition it holds all the mapnames and std::vector<vector2> currentmap; which holds the current map to be rendered on the screen.

[xenovia12]

@Cubbi @doug4 hello so i ended up doing this design
http://pastebin.com/zQWxuLpd
would you tell me if its good?