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
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
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
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
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.
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.