5
\$\begingroup\$

I have a class that reads and writes data to and from a .txt file using fstream.

As far as I can tell it does exactly what I want it to do:

  • if no file exists create one with default initial variables (those are the constants.)
  • if a file does exist read it and set the values of the data structure to those in the file.
  • overwrite the file with data when called.
  • I even left myself a little reset feature that I can use to allow the player to reset their data and start from scatch (although deleting the file would work too)

I don't check that the file hasn't been tampered with but I intend to have those values checked before passing them to their objects.

saveManager.h:

#pragma once
#ifndef SAVEMANAGER
#define SAVEMANAGER
#include <fstream>
#include "saveData.h"

class saveManager
{
public:
    saveManager();

    void read(saveData& data);

    void reset();

    void write(saveData& data);
private:
    // default magic numbers for initializing data

    const int m_major = 1;
    const int m_minor = 1;
    const int m_highWorld = 1;
    const int m_highSubWorld = 1;
    const int m_loot = 0;
    const int m_batSpeed = 4;
    const int m_batSize = 64;
    const int m_maxBallSpeed = 18;
    const int m_ballSize = 10;
    const int m_ballStrength = 1;
    const int m_homing = 0;
    const int m_value = 1;
    const int m_magnet = 30;
    const int m_maxLives = 2;
    const int m_bossDmg = 1;
    const int m_phantom = 0;
    const int m_TBD = 0;

    std::fstream saveFile;
};
#endif // !SAVEMANAGER

saveManager.cpp:

#include "saveManager.h"

saveManager::saveManager()
{
    // do nothing
}

void saveManager::read(saveData& data)
{
    int major;
    int minor;
    saveFile.open("saveData.txt");
    if (saveFile.is_open())
    {
        saveFile >> major >> minor;
        if (major == data.major && minor >= data.minor)
        {
            saveFile >> data.highWorld >> data.highSubWorld >> data.loot >> data.batSpeed >> data.batSize >> data.maxBallSpeed >> data.ballSize >> data.ballStrength >> data.homing >> data.value >> data.magnet >> data.maxLives >> data.bossDmg >> data.phantom >> data.TBD;
        }
        // handle older versions here as needed
    }
    else
    {
        saveFile.open("saveData.txt", std::fstream::in | std::fstream::out | std::fstream::trunc);
        reset();
    }
    saveFile.close();
}

void saveManager::reset()
{
    if (!saveFile.is_open())
    {
        saveFile.open("saveData.txt");
        saveFile << m_major << " " << m_minor << " " << m_highWorld << " " << m_highSubWorld << " " << m_loot << " " << m_batSpeed << " " << m_batSize << " " << m_maxBallSpeed << " " << m_ballSize << " " << m_ballStrength << " " << m_homing << " " << m_value << " " << m_magnet << " " << m_maxLives << " " << m_bossDmg << " " << m_phantom << " " << m_TBD;
        saveFile.close();
    }
    else
    {
        saveFile << m_major << " " << m_minor << " " << m_highWorld << " " << m_highSubWorld << " " << m_loot << " " << m_batSpeed << " " << m_batSize << " " << m_maxBallSpeed << " " << m_ballSize << " " << m_ballStrength << " " << m_homing << " " << m_value << " " << m_magnet << " " << m_maxLives << " " << m_bossDmg << " " << m_phantom << " " << m_TBD;
        saveFile.close();
    }
}

void saveManager::write(saveData& data)
{
    if (!saveFile.is_open())
    {
        saveFile.open("saveData.txt");
        saveFile << data.major << " " << data.minor << " " << data.highWorld << " " << data.highSubWorld << " " << data.loot << " " << data.batSpeed << " " << data.batSize << " " << data.maxBallSpeed << " " << data.ballSize << " " << data.ballStrength << " " << data.homing << " " << data.value << " " << data.magnet << " " << data.maxLives << " " << data.bossDmg << " " << data.phantom << " " << data.TBD;
        saveFile.close();
    }
    else
    {
        saveFile << data.major << " " << data.minor << " " << data.highWorld << " " << data.highSubWorld << " " << data.loot << " " << data.batSpeed << " " << data.batSize << " " << data.maxBallSpeed << " " << data.ballSize << " " << data.ballStrength << " " << data.homing << " " << data.value << " " << data.magnet << " " << data.maxLives << " " << data.bossDmg << " " << data.phantom << " " << data.TBD;
        saveFile.close();
    }
}

I almost forgot to add the data.

saveData.txt:

1 1 1 1 0 4 64 18 10 1 0 1 30 2 1 0 0

I would love any and all feedback. Thank you.

\$\endgroup\$
3
\$\begingroup\$

Design

const int m_major = 1;
const int m_minor = 1;
const int m_highWorld = 1;
const int m_highSubWorld = 1;
const int m_loot = 0;
const int m_batSpeed = 4;
const int m_batSize = 64;
const int m_maxBallSpeed = 18;
const int m_ballSize = 10;
const int m_ballStrength = 1;
const int m_homing = 0;
const int m_value = 1;
const int m_magnet = 30;
const int m_maxLives = 2;
const int m_bossDmg = 1;
const int m_phantom = 0;
const int m_TBD = 0;

These numbers have no, I repeat, no business to be where they are right now.

This kind of one-class-does-everything scheme that your code follows is going to fall on your feet sooner or later, but rather sooner. Let us just imagine you keep working on the game and add more and more functionality. As of right now, you have about two separate default values per functionality, i.e. pairs such as m_batSpeed and m_batSize, m_major and m_minor etc. Now imagine you add some functionality to your game that adds about five different new entities. That means about 10 new constants on average, just in this class!

I hope you realize that this is not sustainable. Even now, there is no way to tell which constant belongs to which actual class besides guessing by name. In a year from now, will you still know what class m_TBD corresponds to, or m_loot? Maybe, but it is more than likely that you will forget some of these. Furthermore, if someone else ever has to or wants to read your source code, they will first be massively stumped, then irritated.

So what can you do about it? Take a serializer-based approach. Usually, when you are writing simple game data, you will have objects corresponding to the current state you want to save. The solution I propose is to approach the classes of those objects and insert a serialize method that writes an object's contents to an std::ostream. To actually save, you just call serialize in order on all objects that need to be preserved.

The second step is, of course, to add a similar deserialize method that takes an std::istream and creates an object from the values read. You then have a file parser read in the file, determine the block sizes, and call those deserializer methods.

The big upside of this solution is that it is relatively easy to implement while allowing you to encapsulate functionality away where it belongs, not in a monster class like you currently have. The downside is that the separation of concerns is still somewhat suboptimal, since you now have what are basically parser methods in game state classes. This can be somewhat mitigated through use of the factory design pattern, for example. Furthermore, this method can become messy if the complexity of the savefile and the game as a whole grows too large, but I am assuming that that is not the case for your project (and if it is, you will have to put a lot of thought in how to handle this well, anyway).

Other Tips and Tricks

  1. Use #pragma once or include guards, not both. They serve the same purpose. If you really care about being standard conforming, stick with the include guards. In most other cases, #pragma once is also fine.

  2. Do not write out default constructors if they don't do anything special, or anything at all. The compiler is nice enough to generate them automatically for you when your don't define any other constructors, which you currently don't.

  3. saveManager is a big class, in terms of object size. One reason for this is that all those ints you define there are part of every object you create and are carried around everywhere your object goes, which is totally unnecessary since those are all constants. You would likely want to make those static at the very least, or, since they are private and inaccessible anyway, even remove them from the class and put them solely into the implementation file as static constants.

    The other reason is that you carry an std::fstream around when you don't need to. In fact, you treat saveFile almost like a local variable: In every method you open the underlying file anew, just to close it before returning. Well, if you do that already, then you don't need to have it as a member variable because there is nothing to preserve here. Just create a new std::fstream every time, which, as an added benefit, will also allow you to get rid of those ugly open() and close() calls and rely on RAII instead.

  4. Your longest line is currently 422 characters long. That is way, way too much. There is no safe and steady rule to what people consider appropriate line lengths, but most programmers agree that lines longer than 100 and a few characters are too long (I personally am a disciple of the traditional 80 columns doctrine). Overly long lines are usually really awkward to work with (just look at the code boxes in your question!), can mess up your terminal, and are generally bothersome when performing actions such as looking at code diffs or merging commits.

  5. Pay attention to const correctness. void write(saveData& data) should be void write(saveData const& data) const (assuming that you also implement the hint about std::fstream I gave in point 3). void read(saveData& data) should be void read(saveData& data) const, or even saveData read() const.

  6. Looking at point 3 and point 5, you can do away with saveManager as a class altogether. You don't need to preserve any state, so your class is effectively empty. Putting read and write as standalone functions would do the job just fine. If you would still like to keep this "these functions belong together"-relation, you could put them in their own namespace.

  7. I don't know whether you care about this or not, but your code is doing no error checking whatsoever. In my opinion, you should at least verify that the file you are reading from or writing to is open and otherwise O.K.

  8. Turn the order of your header includes around. Always include headers from the project you are working on first, then all other headers, including those part of the standard library. This serves the purpose of header verification: If any of your headers are missing an include, your compiler will tell you. If you happen to include any of those missing headers before, however, the code will compile just fine, hiding a bug.

\$\endgroup\$
  • \$\begingroup\$ I do care about error checking. Assume Im taking the rest of the advice you laid out and am rewriting, how should i best approach error checking? \$\endgroup\$ – AGirlHasNoName Mar 7 '18 at 21:39
  • \$\begingroup\$ @bruglesco First off all, you should always verify your fstreams, by simply doing if (file) { // do stuff }, or if (!file) { // error handling goes here}. Actually verifying that what your file contains is valid is much harder. Think about how your data is laid out and what values are allowed where. The easiest check you can do here is verifying the length, because that's something you can compute a lower bound for beforehand. \$\endgroup\$ – Ben Steffan Mar 7 '18 at 21:44

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.