Design of a "storage object" - object used soley to store data

Question

I have a file editor. It reads from a file, creates an object with the read data and can then write that data into a same (or another) file.

Every time user opens a file a new tab is created and a private instance of the object is assigned to it, containing it in the tab.

The object itself is complex and is composited of other objects. Each custom type has a Write() and Read() methods that extract and save data from and into the file. Something like this:

TopObject a0
 int a
 int b
 string c
 CustomObjectMid d
 int e
 bool f
 List<CustomObjectMid2> g
 CustomObjectMid3 h
 CustomObjectBottom i
 long j
 ulong k
 Dictionary<string, int> l
 byte m

The real object is much bigger and has more depth (levels) to it.

TopObject, CustomObjectMid, CustomObjectMid2, CustomObjectMid3 and CustomObjectBottom all have a Write() and Read() methods that populate all standard types with data from file. They don't contain any other methods. They don't even have constructors (other than the default).

The sole purpose of this object is to hold information. It's essentially a structure. This is actually written as a DLL which purpose it to enable users to make sense of that particular file format (read from it and write to it). So the actual code is not in my program, but is a part of a library. As such, I'd like to keep the library relatively "neutral" - I don't want to implement code that is specific to the editor into the library.

The way this object is implemented is that everything is public and I can access anything in a0.h.i.j manner. I tend to pass relevant parts of the objects to pieces of program that use it, so I don't have to reference to everything from the top (a0).

Other parts of the program display the data to the user and translate the changes user makes into the object. They also perform the necessary checks to keep the integrity of data (prevent passing of chars for ints, etc)

My question is is this good design? I decided not to use getters and setters. It would just bloat the code for no reason (at least I can't see one).

All that being said, for some reason this smells like a bad code to me (not sure why), but I don't really see any other way of implementing it without doing an overkill, given it's role in the program.

EDIT: Here is the full code of the root class. All the other classes have the same structure:

namespace SaveManipulator.GameData
{
[Serializable]
public class CustomFile
{
 //version = 7
 public Value<uint> saveFileVersion;
 //ecd
 public ECD ecd;
 //food
 public Food food;
 //drink
 public Drink drink;
 //inventory
 public Stack[] inventory;
 //selectedSlot
 public Value<int> selectedSlot;
 //bag
 public Stack[] bag;
 //craftedList
 public HashSet<string> craftedList;
 //spawnPoints
 public List<Vector3Di> spawnPoints;
 //SpawnKey
 public Value<long> SpawnKey;
 //notSaved = true
 public Value<bool> randomBoolean;
 //notSaved = 0
 public Value<short> randomShort;
 //bLoaded
 public Value<bool> bLoaded;
 //lastPosition
 public Vector3Di lastPosition;
 //id
 public Value<int> id;
 //droppedPosition
 public Vector3Di droppedPosition;
 //kills
 public Value<int> kills;
 //fKills
 public Value<int> fKills;
 //deaths
 public Value<int> deaths;
 //score
 public Value<int> score;
 //equipment
 public Equipment equipment;
 //unlockedList
 public List<string> unlockedList;
 //notSaved = 1
 public Value<ushort> randomUShort;
 //marker
 public Vector3Di marker;
 //favorites
 public Equipment favorites;
 //experience
 public Value<uint> experience;
 //level
 public Value<int> level;
 //bCrouched
 public Value<bool> bCrouched;
 //cdata
 public Cdata cdata;
 //favoriteList
 public List<string> favoriteList;
 //J
 public Skills skills;
 //totalItems
 public Value<uint> totalItems;
 //distance
 public Value<float> distance;
 //life
 public Value<float> life;
 //waypoints
 public Waypoints waypoints;
 //skillPoints
 public Value<int> skillPoints;
 //quests
 public Quests quests;
 //deathTime
 public Value<int> deathTime;
 //currentL
 public Value<float> currentL;
 //bDead
 public Value<bool> bDead;
 public CustomFile Clone()
 {
 Stream stream = new FileStream("CustomFile ", FileMode.Create, FileAccess.Write, FileShare.None);
 IFormatter formatter = new BinaryFormatter();
 formatter.Serialize(stream, this);
 stream.Close();
 stream = new FileStream("CustomFile", FileMode.Open, FileAccess.Read, FileShare.None);
 PlayerDataFile clone = (PlayerDataFile)formatter.Deserialize(stream);
 stream.Close();
 File.Delete("CustomFile");
 return clone;
 }
 public void Read(string path)
 {
 BinaryReader reader = new BinaryReader(new FileStream(path, FileMode.Open));
 if (reader.ReadChar() == 'd' && reader.ReadChar() == 't' && reader.ReadChar() == 's' &&
 reader.ReadChar() == '0円')
 {
 saveFileVersion = new Value<uint>((uint)reader.ReadByte());
 ecd = new ECD();
 ecd.Read(reader);
 food = new Food();
 food.Read(reader);
 drink = new Drink();
 drink.Read(reader);
 inventory = Stack.Read(reader);
 selectedSlot = new Value<int>((int)reader.ReadByte());
 bag = Stack.Read(reader);
 //num
 int craftedListLength = (int)reader.ReadUInt16();
 craftedList = new HashSet<string>();
 for (int i = 0; i < craftedListLength; i++)
 {
 craftedList.Add(reader.ReadString());
 }
 //b
 byte spawnPointsCount = reader.ReadByte();
 spawnPoints = new List<Vector3Di>();
 for (int j = 0; j < (int)spawnPointsCount; j++)
 {
 Vector3Di spawnPoint = new Vector3Di();
 spawnPoint.x = new Value<int>(reader.ReadInt32());
 spawnPoint.y = new Value<int>(reader.ReadInt32());
 spawnPoint.z = new Value<int>(reader.ReadInt32());
 spawnPoints.Add(spawnPoint);
 }
 spawnKey = new Value<long>(reader.ReadInt64());
 randomBoolean = new Value<bool>(reader.ReadBoolean());
 randomShort = new Value<short>(reader.ReadInt16());
 bLoaded = new Value<bool>(reader.ReadBoolean());
 lastPosition = new Vector3Di();
 lastPosition.x = new Value<int>(reader.ReadInt32());
 lastPosition.y = new Value<int>(reader.ReadInt32());
 lastPosition.z = new Value<int>(reader.ReadInt32());
 lastPosition.heading = new Value<float>(reader.ReadSingle());
 id = new Value<int>(reader.ReadInt32());
 droppedPosition = new Vector3Di();
 droppedPosition.x = new Value<int>(reader.ReadInt32());
 droppedPosition.y = new Value<int>(reader.ReadInt32());
 droppedPosition.z = new Value<int>(reader.ReadInt32());
 kills = new Value<int>(reader.ReadInt32());
 fKills = new Value<int>(reader.ReadInt32());
 deaths = new Value<int>(reader.ReadInt32());
 score = new Value<int>(reader.ReadInt32());
 equipment = Equipment.Read(reader);
 //num
 int count = (int)reader.ReadUInt16();
 unlockedList = new List<string>();
 for (int k = 0; k < count ; k++)
 {
 unlockedList .Add(reader.ReadString());
 }
 randomUShort = new Value<ushort>(reader.ReadUInt16());
 marker = new Vector3Di();
 marker.x = new Value<int>(reader.ReadInt32());
 marker.y = new Value<int>(reader.ReadInt32());
 marker.z = new Value<int>(reader.ReadInt32());
 favorites = Equipment.Read(reader);
 experience = new Value<uint>(reader.ReadUInt32());
 level = new Value<int>(reader.ReadInt32());
 bCrouched = new Value<bool>(reader.ReadBoolean());
 cdata = new Cdata();
 cdata.Read(reader);
 //num
 int favoriteListSize = (int)reader.ReadUInt16();
 favoriteList = new List<string>();
 for (int l = 0; l < favoriteListSize; l++)
 {
 favoriteList.Add(reader.ReadString());
 }
 //num2
 int size = (int)reader.ReadUInt32();
 skills = new Skills();
 skills.Read(reader);
 totalItems = new Value<uint>(reader.ReadUInt32());
 distance = new Value<float>(reader.ReadSingle());
 life = new Value<float>(reader.ReadSingle());
 waypoints = new Waypoints();
 waypoints.Read(reader);
 skillPoints = new Value<int>(reader.ReadInt32());
 quests = new Quests();
 quests.Read(reader);
 deathTime = new Value<int>(reader.ReadInt32());
 currentL = new Value<float>(reader.ReadSingle());
 bDead = new Value<bool>(reader.ReadBoolean());
 //irelevant bytes
 reader.ReadByte();
 reader.ReadBoolean();
 reader.Close();
 }
 else
 {
 throw new IOException("Save file corrupted!");
 }
 }
 public void Write(string path)
 {
 BinaryWriter writer = new BinaryWriter(new FileStream(path, FileMode.Create));
 writer.Write('d');
 writer.Write('t');
 writer.Write('s');
 writer.Write((byte)0);
 writer.Write((byte)saveFileVersion.Get());
 ecd.Write(writer);
 food.Write(writer);
 drink.Write(writer);
 Stack.WriteItemStack(writer, inventory);
 writer.Write((byte)selectedSlot.Get());
 Stack.WriteItemStack(writer, bag);
 writer.Write((ushort)craftedList.Count);
 HashSet<string>.Enumerator enumerator = craftedList.GetEnumerator();
 while (enumerator.MoveNext())
 {
 writer.Write(enumerator.Current);
 }
 writer.Write((byte)spawnPoints.Count);
 for (int i = 0; i < spawnPoints.Count; i++)
 {
 writer.Write(spawnPoints[i].x.Get());
 writer.Write(spawnPoints[i].y.Get());
 writer.Write(spawnPoints[i].z.Get());
 }
 writer.Write(selectedSpawnKey.Get());
 writer.Write(randomBoolean.Get());
 writer.Write(randomShort.Get());
 writer.Write(bLoaded.Get());
 writer.Write((int)lastPosition.x.Get());
 writer.Write((int)lastPosition.y.Get());
 writer.Write((int)lastPosition.z.Get());
 writer.Write(lastPosition.heading.Get());
 writer.Write(id.Get());
 writer.Write(droppedPosition.x.Get());
 writer.Write(droppedPosition.y.Get());
 writer.Write(droppedPosition.z.Get());
 writer.Write(kills.Get());
 writer.Write(zKills.Get());
 writer.Write(deaths.Get());
 writer.Write(score.Get());
 equipment.Write(writer);
 writer.Write((ushort)unlockedList.Count);
 List<string>.Enumerator enumerator2 = unlockedList.GetEnumerator();
 while (enumerator2.MoveNext())
 {
 writer.Write(enumerator2.Current);
 }
 writer.Write(randomUShort.Get());
 writer.Write(marker.x.Get());
 writer.Write(marker.y.Get());
 writer.Write(marker.z.Get());
 favorites.Write(writer);
 writer.Write(experience.Get());
 writer.Write(level.Get());
 writer.Write(bCrouched.Get());
 cdata.Write(writer);
 writer.Write((ushort)favoriteList.Count);
 List<string>.Enumerator enumerator3 = favoriteList.GetEnumerator();
 while (enumerator3.MoveNext())
 {
 writer.Write(enumerator3.Current);
 }
 skills.Write(writer);
 writer.Write(totalItems.Get());
 writer.Write(distance.Get());
 writer.Write(life.Get());
 waypoints.Write(writer);
 writer.Write(skillPoints.Get());
 quests.Write(writer);
 writer.Write(deathTime.Get());
 writer.Write(currentL.Get());
 writer.Write(bDead.Get());
 writer.Write((byte)88);
 writer.Write(true);
 writer.Close();
 }
 public CustomFile(string path)
 {
 Read(path);
 }
}
}

Since the code is mostly copied from the source of the game, I obfuscated it a little bit, that's why some names may look vague. Also Value<Type> is a wrapper class used to pass around value types while keeping their reference without explicitly having to think about it. That way the user can change bits of the file easier. I don't really see how I could partially save the file. Save files are usually from 4kB to (my estimate) 10kB. They aren't that big.

Answer 1

Oh dear, what has become of the world, when people are worried about using a simple structure?

Yes, this is perfectly fine. Your code does not smell. Structures are just another name for record types (although using the word structure usually connotes mutability), which are a form of product type. Product types are one of the two fundamental composite data types in programming languages / type systems (the other being sum types). The more you can rely on these simple types, the better. Getters and setters are absolutely overkill.

For what it's worth, objects are just record types where some of the fields are functions (with a sprinkling of self-reference to keep things adequately confusing). Classes are just a type-alias for a certain record type along with a function (called the constructor) that creates records of that type.

Answer 2

I wouldnt say its bad, but it would be very hard to program well.

The key problem comes around the requirement to save/read only parts of the object. Leading to the read/write methods in each object rather than soley on the root object.

You can imagine the problems, for example I see you have a list g under the root object. If I create a new g and add it to the list, does that auto save it? Do i need to call g.save() or root.save() or both?

Can I load a sub object without first loading its parent?

It seems like this library is following an active record pattern. A more modern approach would be to only save/load the top level 'Aggregate root' object.

You would probably write a seperate repository object to Handle the file format and writing to disk etc.

However active record was all the rage a few years ago, and if the requiremenr to load/save parts of the root object is a must, then you would need to write something like this. I know dome people take a very hard line approach to 'code smell' I am a bit more forgiving. I would say yes its a code smell, but if you investigate you might find there is a good reason for it!

In any case since you are consuming the library you dont have a whole lot of choice in the matter.

My gut reaction is to try and wrap it in a repository style of save /load, iterating through the tree and saving all the items. Only passing the root into your own functions etc but without a full understanding its a tough call to assume this would be a sensible use of time.

Are you writing lots of duplicate 'save all g and childern' 'load all d where x' style logic? Have you found a bug where you need to check the file for a parent before you know child.save() will execute correctly? or does it 'just work'?

Edit ****

Sorry I just realised its a library which you wrote yourself. Definitely rewrite as just a struct in a Models dll plus a FileRepository in a second dll. Then people can consume the object without the saving logic and you can add additional DatabaseRepository, InMemoryRespository etc etc later

• Karlovsky120: The thing is that it's a savegame file. I'm updating the question with a sample of root class. The class doesn't really make much sense if I don't add the save/load code. The whole object is saved by calling rootInstance.Write(string path) which then deals with all the rest of the saving. Since it's a binary file, I don't think inserting is a thing that would speed up anything. Is it?
• Ewan: i would totaly split it into GameState and GameStateRepository. The key thing I notice is your Equipment.Read() method. Say you change the format to xml, or download a save from the internet. You dont want Equipment to know about binary writer
• Ewan: Also, game are notorious for breaking changes in save games. Putting the parsing logic in a repository allows you to have GameStateRepoV1 etc