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