C#: Structuring Saved Data for a Game

Question

I'm working on the structure of the fields that are persistent in my Unity game. Things like inventory, etc. It is saved by serializing a class and saving to disk (as explained here). The problem I'm having is figuring out the best practice to structure it all. I want 2 classes that can access it - one that checks and modifies the values during gameplay and another that actually saves and loads the data to disk.

To have good object oriented design, I thought about trying to make the saved data a private, nested class but the saving is multithreaded and has 9 methods so it really seems like it should be a separate class - and if 2 classes access the saved data class then it has to be public, with all of its variables being public as well... which just seems like a bad design. Also, I have a separate class from the get/set methods for the variables because it should be saving to disk a lot and the excessive amount of get and set methods will add up to make the file much larger...

I'm really not sure what the best way of structuring this is, and I don't want to have to make a ton of changes later on if I choose the wrong structure. Any thoughts and opinions would be appreciated. Thanks.

public class SaveData {
    public float health;
    public float energy;
    public int currentLevel;
    public int bullets;
    .
    .
    .
}

Class that gets/sets data, seems almost pointless if it is public (besides tracking bugs, and possibly future checks to make sure values are legit

public class DataController : MonoBehaviour {
    [SerializeField]SaveController saveController;
    SaveData data;

    public void SetEnergy(float newEnergy)
    {
        data.energy = newEnergy;
    }

    public float GetEnergy()
    {
        return data.energy;
    }
    .
    .
    .
}

Class that saves the data to file

public class SaveController : MonoBehaviour {

    [SerializeField]DataController dataController;
    Thread timeSaveThread;
    Thread pickUpSaveThread;

    // Use this for initialization
    void Start () {
       timeSaveThread = new Thread(BeginTimeSave);
       timeSaveThread.Start();
    }

    public void StartPickupThread(){

    }

    void BeginTimeSave()//save every x minutes
    {
    }

    void BeginPickUpSave()//save right after an item is picked up
    {
    }

    void BinarySave()
    {

    }
    .
    .
    .
}

Answer 1

I want 2 classes that can access it - one that checks and modifies the values during gameplay and another that actually saves and loads the data to disk.

I am all for segregating single responsibilities; but I'm hardpressed to consider checking and modifying values as a responsibility (getting/setting is the core of how OOP objects communicate).

All you really need is a way to load/save the SaveData object. Accessing the data (when it is in-memory) is simply part of the SaveData class (by exposing properties).

Separating the save/load logic is a good idea; but I can see an argument for considering it overengineering, if the save/load logic is trivial.
Either way, I suggest you either implement the save/load methods inside the SaveData class definition; or you abstract it in a separate class (e.g. SaveDataLoader) which I would expose as a property of SaveData (e.g. mySaveData.Loader.SaveToDisk());

To have good object oriented design, I thought about trying to make the saved data a private, nested class

What is the point of a class, intended to hold data, which doesn't expose public access to the data it's supposed to be holding? It seems to me that it defeats the purpose of having a data class in the first place.

but the saving is multithreaded and has 9 methods so it really seems like it should be a separate class

Multithreaded reading is not really a problem (no race conditions). Multithreaded writing is more of a problem; but this can easily be solved by using a lock.

How does the saving (to disk) have 9 different methods? I can see why you'd have different methods for changing a value, but I'd expect there to be only one method that's responsible for writing the save data to disk.

and if 2 classes access the saved data class then it has to be public,

The number of classes (that access the save data) has no bearing on whether or not it should be public or private. The only thing that matters is if you want an external caller to have access to the data (if yes, then you need to make it public). Whether it's 1, 2 or 250 external callers is irrelevant.

with all of its variables being public as well

Maybe a bit pedantic, but you only need to expose the properties that you want external callers to have access to. There's nothing wrong with having some private fields for logical purposes that do not contain relevant information for external callers.

... which just seems like a bad design.

I don't quite agree. Public access to properties is not a code smell. Making all properties of all classes public by default in your entire assembly is a code smell; but that's not what we're talking about here.

A DTO's data properties should logically be public, so that external callers are able to actually interact with the DTO.

Given the need for thread-safety, you may want to wrap your DTO in a class that streamlines the multithreaded calls (e.g. if it uses a lock to ensure that only one caller accesses the DTO object at the same time).
But nonetheless, both the DTO and its wrapper class would need to have public properties/methods in order to be used by external callers.

Class that gets/sets data, seems almost pointless if it is public

As it is currently implemented, the class is a bit pointless. However, you can implement the lock logic here (to make sure that only one thread accesses the save data at the same time)

---

void BeginTimeSave()//save every x minutes
{
}

void BeginPickUpSave()//save right after an item is picked up
{
}

I'm a bit puzzled as to why these are two separate methods. I would expect there to be a single Save() method; which is called from multiple locations (e.g. both the OnTimerExpired and OnItemPickedUp events).

I may be misunderstanding the intention of the methods; but then I think you've misnamed them. If BeginTimeSave is responsible for setting a timer that will trigger a save when it expires; you should rename it to something like BeginSaveTimer, since it doesn't actually include the logic of saving the data (I assume that's what BinarySave() does).

Note: I would personally separate the save/load logic and the triggers that decide when to save/load. Those are two separate responsibilities. Even though one will call the other, they share no common logic in and of themselves.

---

Summary

Your intentions are good. You're aware of thread safety and try to adhere to SOLID principles (mainly SRP in this case).

However, I think you're somewhat overengineering your approach to exposing public access to your DTO. I see no reason to avoid public access to a DTO's properties, as access to the data is a DTO's primary purpose.
Other than the need to ensure thread safety, I think you're creating more obstacles than are necessary. Simply implement a thread-safe wrapper around your DTO and you should be good to go.