C++ Saving And Loading Data For A Game To a Text File Using fstream

Question

So i have written this code to save and load data for my C++ game to a file, this way I don't have to hard code every single room and item for the game. Essentially here's how the game works: So you have a Map, and a map consists of rooms. A room contains tiles and items. there are 32x18 tiles in a room, and a tile is just a 40x40 image basically, and each tile has a unique numeric ID. An item works similarly, however, it can be at any coordinate in the room and isn't necessarily 40x40 pixels, it has a sprite, and a unique numeric ID, and of course data for it's position in the room (the tiles don't need this because their index in the tiles matrix act as their coordinates, just multiply it's indexes by 40 and you have it's coordinates for screen). There is a RoomBuilder class which my saving and loading feature is in, and this allows me as the developer to click on the screen and place tiles/items which will be added to the current room i'm in. If you need further clarification on how a room system works, think original Legend of Zelda on the NES, how you are in a room then when you go off the screen you are in another room pretty much.

What I do is I Save the data as a string to the output file like this: A line will have Tiles: at the beginning to signify that it is the tiles for that room, there are 576 numbers on that line each separated by a space (32x18 tiles per room = 576). It starts at the top left and saves them row by row, left to right.

When I read the line with Tiles, i save each number into a vector, then loop through that vector at the beginning and set each tile ID in the room based on that vector, so it will start at index 0 in the vector, get that number, go all the way to 32 and save those as the first row of tiles, then go to 64 from there and save those as the second row of tiles, and so on.

For the items, each item in the room has it's own line, it starts with ID: to signify that it's an item, and there are 3 numbers on an item's line in the text file; the first one is it's numeric ID, the second one is it's X position, the third one is it's Y position. I read that, and add an item to the room's vector of items based on the item's ID and position.

The code works, but i'm not happy with it because it's ugly, very finicky, could have bugs I don't know about, and there are parts that I don't fully understand about it (mainly the string stream in the loading method where i have a temporary string and then parse each integer out, I found that code online and tested it, it worked great for my purposes but the point of this project is to learn, and i'm not learning from that since I don't understand it). If I change anything slightly that could break the whole loading/saving system and it will cause a segfault error if that breaks because now I am in a room that doesn't exist since I wasn't able to load any of the rooms.

Feedback is greatly appreciated, I'm looking for things I did wrong with this code, things that could cause it to break, and some tips on how I could improve it, specifically formatting the output and reading it easier.

Saving Method:

bool RoomBuilder::saveRooms(){
 //open the output file to save the rooms to:
 std::ofstream out("GameData\\DefaultMapRoomBuilder.klvl");
 //loop through each room in the rooms vector
 for(int i = 0; i < rooms.size(); i++){
 //write the tiles matrix for this room to the file:
 out << "Tiles: ";
 for(int j = 0; j < 32; j++){
 for(int k = 0; k < 18; k++){
 out << rooms.at(i).getTile(j, k) << " ";
 }
 }
 out << "\n";
 //save the items and their positions to the room:
 for(int j = 0; j < rooms.at(i).items.size(); j++){
 out << "Items: \n";
 int itemID = rooms.at(i).items.at(j).getItemID();
 float xPos = rooms.at(i).items.at(j).getPosition().x;
 float yPos = rooms.at(i).items.at(j).getPosition().y;
 out << "ID: " << itemID << " " << xPos << " " << yPos << std::endl;
 }
 //text to signify end of current room:
 out << "END_OF_ROOM" << "\n";
 }
 return true;
}

loading method:

bool RoomBuilder::loadRooms(){
 std::ifstream in("GameData\\DefaultMapRoomBuilder.klvl");
 std::vector<std::string> lines;
 //only happens if input stream is open:
 if(in.is_open()){
 //string to store the current line we're on:
 std::string line;
 //while loop to loop through each line:
 while(std::getline(in, line)){
 //add each line to the lines vector:
 lines.push_back(line);
 }
 //close the file now that we no longer need it:
 in.close();
 }
 rooms.push_back(Room());
 int roomIndex = 0;
 for(int i = 0; i < lines.size(); i++){
 std::cout << lines.at(i) << std::endl;
 if(lines.at(i).find("END_OF_ROOM") != std::string::npos){
 if(i != lines.size()-1)
 rooms.push_back(Room());
 roomIndex++;
 std::cout << "loaded room at iteration: " << i << std::endl;
 }
 else if(lines.at(i).find("Tiles:") != std::string::npos){
 std::vector<int> tiles;
 std::stringstream ss;
 ss << lines.at(i);
 std::string temp;
 int found;
 while(!ss.eof()){
 ss >> temp;
 if(std::stringstream(temp) >> found)
 tiles.push_back(found);
 temp = "";
 }
 int tIndex = 0;
 for(int x = 0; x < 32; x++){
 for(int y = 0; y < 18; y++){
 rooms.at(roomIndex).setTile(x, y, tiles[tIndex]);
 tIndex++;
 }
 }
 }
 else if(lines.at(i).find("ID:") != std::string::npos){
 std::vector<int> itemInfo;
 std::stringstream ss;
 ss << lines.at(i);
 std::string temp;
 int found;
 while(!ss.eof()){
 ss >> temp;
 if(std::stringstream(temp) >> found)
 itemInfo.push_back(found);
 temp = "";
 }
 Item I = *Materials::getItem(itemInfo[0]);
 I.setPosition(sf::Vector2f(itemInfo[1], itemInfo[2]));
 rooms.at(roomIndex).addItem(I);
 std::cout << "added item at iteration: " << i << std::endl;
 }
 }
 return true;
}
```

• Loki Astari: I would use a common serialization convention like YAML or JSON. Then you don't need to worry about the serialization and deserialization so much as this can be done via a standard library.

Answer 1

Although most of the code is not shown, which makes the code much harder to review, by the way, here are some observations that may help you improve your code.

Use const where practical

The saveRooms member function of RoomBuilder does not appear to alter the underlying object and therefore should be declared const.

bool RoomBuilder::saveRooms() const {
 // code here
}

Don't hardcode file names

In maintaining this code later, the name of the output file might be something that could be placed elsewhere. Right now, it appears to have a Windows-centric name but if you wanted to port this to Mac or Linux, it would probably be better to pick out the file name as a named compile-time constant like this:

constexpr const char *roomFileName{"DefaultMapRoomBuilder.klvl"};

That way both the loadRooms and saveRooms could refer to exactly the same file without having to type the filename twice. Better might be to pass the file name as a parameter to both functions.

Don't write getters and setters for everything

There are a lot of functions like Item::setPosition and Item::getPosition that suggest you're attempting to write Java in C++. Don't do that. If other classes truly require unfettered access to the details of another class, just make it a struct and directly set and fetch the contents.

Use appropriate data structures

In the description of your code, you write that the rooms have a fixed number of tiles, but your code does not seem to reflect this directly, using a std::vector instead of a std::array. If you're relying on a fixed size (as the code appears to do) then use a fixed size data structure as well. If not, then the code would need to be altered to somehow account for variable sized data.

Rethink item numbering

The current code contains these lines:

int tIndex = 0;
for(int x = 0; x < 32; x++){
 for(int y = 0; y < 18; y++){
 rooms.at(roomIndex).setTile(x, y, tiles[tIndex]);
 tIndex++;
 }
}

This probably works just fine for you, but the usual order of storing this is the other way around. That is, the inner loop would more typically be the x loop.

Use standard library functions

Here's how I would write saveRooms:

bool RoomBuilder::saveRooms() const {
 std::ofstream out{roomFileName};
 std::copy(rooms.begin(), rooms.end(), std::ostream_iterator<Room>(out, "\n"));
 return static_cast<bool>(out);
}

There's a lot going on here in just a few lines. First note that I'm using the C++11 uniform initialization syntax to initialize the variable.

The last line takes advantage of the fact that std::ofstream has an explicit operator bool() that returns the state of the stream. (Some people would write return !!out; but I think my version is more clear in intent.)

The std::copy is an elegant function that allow you to use an already defined extractor for Room. Here's one way to write that as a friend function of Room:

friend std::ostream& operator<<(std::ostream& out, const Room& r) {
 out << "Tiles: ";
 std::copy(r.tile.begin(), r.tile.end(), std::ostream_iterator<int>(out, " "));
 out << '\n';
 std::copy(r.items.begin(), r.items.end(), std::ostream_iterator<Item>(out, "\n"));
 return out << "END_OF_ROOM";
}

This, in turn, relies on a similar function for Item:

friend std::ostream& operator<<(std::ostream& out, const Item& item) {
 return out << "Items: \nID: " << item.id << " " << item.pos.x << " " << item.pos.y;
}

I have exactly reproduced the behavior of the original in which the "Items:" line is repeated for each item. If this were my code, I'd probably either change that to only write it once, or eliminate it entirely, since the "ID:" string is enough to uniquely identify the item in this context.

Rethink the interface

In the same logic as the rewritten function above, in reality, I probably wouldn't have loadRooms and saveRooms functions, but instead define ostream and istream functions like the ones described above for Item and Room.

Do more error handing

If the input file is malformed, there is very little error checking in the existing code. I'd suggest instead that the code throw if an error is found in the formatting or contents of the input file.

Avoid pointless complexity

The use of lines in loadRooms() makes little sense and only contributes complexity to the code. One could much more simply read and process each line directly from the file.

• KevinCMD: Thanks a lot, this was very helpful. I scrapped the entire roombuilder class and named it's replacement WorldMap, i made ostream operators for Room, Item, and now Player. I changed the vector to a 2d array so i was able to scrap a ton of code related to room's neighbor rooms in game. I didn't know about std::copy before, it was helpful. I added the file name to WorldMap's constructor. I made a lot of stuff a constant global variable, and now changing the resolution works! Thats all i had time to do today but i spent all of it just refactoring my code instead of just slapping new features on.
• Edward: I'm glad you found it useful. Some of my most productive days have been spent in just refactoring code. One thing that may help is to also have automated unit tests. I find that having such tests helps me refactor code with confidence and efficiency.

Answer 2

You have not provided full details, so let me go on partly guessing here. First, here's a few suggestions that you can likely do immediately.

• Both methods return a boolean, but it's always true. What's even worse, it seems that loadRooms() return true even if you were unable to open the stream! Probably you intended to return false in both cases in case of any I/O failures.

• You can avoid human errors by using proper constants. Don't use magic numbers 32 and 18, but rather make them constants like const int ROOM_WIDTH and const int ROOM_HEIGHT at a suitable place.

• Because itemID, xPos and yPos can be const, make them such. This helps the reader see that the variables won't change later on, reducing mental effort.

• In saveRooms(), you might only want to retrieve rooms.at(i).items.at(j).GetPosition(), and then print out x and y to avoid making two function calls to the getter.

• In both methods, you have hard-coded the name of the file resource. Why? Instead, consider taking that as an input to the constructor to RoomBuilder, again reducing the chance of errors.

• In loadRooms(), you could first check if the stream is OK, and if not, then return false. Otherwise, you could apply IIFE to initialize const std::vector<std::string> lines appropriately.

• I think loadRooms() is doing too much. Why not separate the logic of reading tiles into one and reading item information into another function? Those can be called from the method then.

• You are not really checking for whether the input is valid. As per your checks, it's fine as long as you find say "ID:" or "Tiles:" anywhere in the string. I would suggest using some well-known structured format like JSON for storing all this information.

• With std::stringstream, you can initialize the object with the correct string input, e.g., std::stringstream ss("id 15 20"); after which you can do say ss >> id >> x >> y;, where id is a string and x and y are ints.

• For checking streams, you can just invoke their operator(), i.e., you can do if(ss) { ... } or if(in) { ... }.

In addition to these points, I would recommend you consider the following more breaking changes.

• Why is the responsibility of RoomBuilder to know how a specific room is saved to a file?

• The above should be the responsbility of every room: they know their details and how to draw itself. That way, the room builder could really just say "for each room r, call r.draw()" and that's it. In that way, rooms can easily be of different size, have different cool special effects, etc.