2
\$\begingroup\$

I made a Json save system for my Unity game. I wanted to make something as generic as possible to keep the code DRY. I couldn't find much information online on how to make something generic.

Could you guys let me know what do you think of it?

So first, I have this utils class to save and load json data.

public class JsonDataManagement
{
    private static string GetPath(string fileName)
    {
        return Application.persistentDataPath + Path.AltDirectorySeparatorChar + fileName;
    }

    public static void SaveData<T>(string fileName, T data)
    {
        string path = GetPath(fileName);

        string jsonString = JsonUtility.ToJson(data);

        using StreamWriter writer = new StreamWriter(path);

        writer.Write(jsonString);
    }

    public static T LoadData<T>(string fileName)
    {
         string path = GetPath(fileName);

         using StreamReader reader = new StreamReader(path);

         string json = reader.ReadToEnd();

         return JsonUtility.FromJson<T>(json);
    }

    public static bool FileExists(string fileName)
    {
        string path = GetPath(fileName);

        return File.Exists(path);
    }
}

Then, I have this container class to store the data. As it is generic, I can pass any class to it:

public class JsonDataUser<T>
{
    public T JsonData;

    private string jsonFileName;

    public void SaveData()
    {
        JsonDataManagement.SaveData<T>(jsonFileName, JsonData);
    }

    public JsonDataUser(T _StartJsonData, string _jsonFileName)
    {
        jsonFileName = _jsonFileName;

    /// <summary>
    /// Currently we only need to load the data once as the game begins
    /// This is why we do the loading here in the constructor and why
    /// we don't have a dedicated load method
    /// </summary>

    if (JsonDataManagement.FileExists(jsonFileName) == true)
    {
        JsonData = JsonDataManagement.LoadData<T>(jsonFileName);
    }

    else
    {
        JsonData = _StartJsonData;
    }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ You're not following the Microsoft naming guidelines, plus your capitalization is inconsistent. \$\endgroup\$
    – BCdotWEB
    Commented Mar 22, 2022 at 10:44
  • 1
    \$\begingroup\$ @BCdotWEB That is an answer. \$\endgroup\$
    – pacmaninbw
    Commented Mar 22, 2022 at 11:37

1 Answer 1

2
\$\begingroup\$

JsonDataManagement

Since you can not enforce the consumer of your class to call FileExists before any other operation call that's why your current implementation is fragile.

I would suggest to make that method private and call it inside the SaveData and LoadData. Also if existence check fails then you should handle it. In my case I throw a FileNotFoundException but you might want to create the file on behalf of the user when the SaveData is called.

public class JsonDataManagement
{
    public static void SaveData<T>(string fileName, T data)
    {
        string path = GetPathAndCheckExistency(fileName);
        using var writer = new StreamWriter(path);
        writer.Write(JsonUtility.ToJson(data));
    }

    public static T LoadData<T>(string fileName)
    {
        string path = GetPathAndCheckExistency(fileName);
        using var reader = new StreamReader(path);
        return JsonUtility.FromJson<T>(reader.ReadToEnd());
    }

    private static string GetPathAndCheckExistency(string fileName)
    {
        var filePath = string.Join("", Application.persistentDataPath, Path.AltDirectorySeparatorChar, fileName);
        if (!File.Exists(filePath))
            throw new FileNotFoundException(message: "The provided file does not exist.", fileName: fileName);
        return filePath;
    }
}

JsonDataUser

I think this name is misleading with the User suffix. For me this functionality resembles to the memento design pattern, so callig the class JsonDataMemento might be a better idea.

I think you should not anticipate the filename from the class consumer. It should be an auto-generated name hidden from the consumer. But depending on the requirements it might make sense to expect that from the user.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Thanks a lot for your help. I am just wondering, why makes it fragile? If there is no file, then the base data will be loaded instead. Same thing for saving, if there is no file yet, a new file based on the current data will be saved. I guess there are some principles I am missing. \$\endgroup\$ Commented Mar 25, 2022 at 13:48
  • \$\begingroup\$ @willywinelover I said it is fragile since the JsonDataManagement class can be used like this: JsonDataManagement.LoadData<T>(jsonFileName) without performing an existence check for the file. So the StreamReader will throw an exception while it tries to read from a non-existing file. Is it clear now? \$\endgroup\$ Commented Mar 25, 2022 at 14:44
  • \$\begingroup\$ @willywinelover According to my understanding the JsonDataManagement can be used without the JsonDataUser wrapper. So placing the fallback logic into a wrapper does not help on the fragileness of the manager class. \$\endgroup\$ Commented Mar 25, 2022 at 14:46
  • 1
    \$\begingroup\$ Ok now I see it. Thanks a lot. \$\endgroup\$ Commented Mar 25, 2022 at 15:03
  • \$\begingroup\$ By the way, when you say "I think you should not anticipate the filename from the class consumer. It should be an auto-generated name hidden from the consumer. But depending on the requirements it might make sense to expect that from the user.". I am not sure how to do this. How is JsonDataManagement supposed to know what each file should be named? \$\endgroup\$ Commented Mar 25, 2022 at 15:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.