4
\$\begingroup\$

I made a class to conviniently serialize/deserialize data. Now I'm stuck with the following question. Should I handle exceptions (use try-catch) inside JsonStorage or inside ConfigManager and other classes that might use JsonStorage in the future?

public class JsonStorage : IDataStorage
{
    private readonly IFileSystem fileSystem;
    private readonly ILogger logger;

    public JsonStorage(IFileSystem fileSystem, ILogger logger)
    {
        this.fileSystem = fileSystem;
        this.logger = logger;
    }

    public T RestoreObject<T>(string path)
    {
        T result = default;

        try
        {
            if (!fileSystem.Path.IsPathFullyQualified(path))
                throw new ArgumentException("Invalid path.", nameof(path));

            string json = fileSystem.File.ReadAllText($"{path}.json");
            result = JsonConvert.DeserializeObject<T>(json);
        }
        catch (Exception ex)
        {
            logger.Log($"Object can't be restored. {ex.Message}");
        }

        return result;
    }

    public void StoreObject(object obj, string path)
    {
        string file = $"{path}.json";

        try
        {
            if (!fileSystem.Path.IsPathFullyQualified(path))
                throw new ArgumentException("Invalid path.", nameof(path));

            fileSystem.Directory.CreateDirectory(fileSystem.Path.GetDirectoryName(file));
            var json = JsonConvert.SerializeObject(obj, Formatting.Indented);
            fileSystem.File.WriteAllText(file, json);
        }
        catch (Exception ex)
        {
            logger.Log($"Object can't be stored. {ex.Message}");
        }
    }
}
public static class ConfigManager
{
    private static readonly string configPath = @"SomePath\Config";

    public static BotConfig LoadConfig()
    {
        return Unity.Resolve<IDataStorage>().RestoreObject<BotConfig>(configPath);
    }

    public static void SaveConfig(BotConfig config)
    {
        Unity.Resolve<IDataStorage>().StoreObject(config, configPath);
    }
}

ConfigManager - example class that holds a path to Config.json and loads/saves it (using JsonStorage internaly).

For now I'm catching exceptions inside JsonStorage and using a logger instance to print exception messages to the console. I don't know if there's a downside to that. Should JsonStorage have its own logger? Should it throw an exception (just like ReadAllText/WriteAllText) when something goes wrong with path/files? Or maybe there is a good practice and some better ways to handle exceptions (in my particular case)?

\$\endgroup\$
8
\$\begingroup\$

This is how I see things...

  • Throwing ArgumentException insinde a try/catch that at the same time swollows it is pointless. You use ArgumentException to inform the user that he called your API in a wrong way. This is not something one should hangle because he won't see it until he looks at the logs. The app should crash in this case.
  • The message of the ArgumentException is too generic. When it happens, the user already knows something is wrong. Tell him how to fix that by giving him a hint what the correct path should be and what path he actually used.
  • The way you are handling exceptions that might be thrown by the json-serializer is unexpected (non-standard) because the user will wonder why he got a null without noticing anything. public library APIs should always throw unless they are called TryDoSomething and return a bool.
  • Yet another flaw of this pattern is that it doesn't help anyone to find the source of the exception. You neither add the filename to it nor the object that it tried to de/serialize. This is very important piece of information and should be included.

So, what you can/should do instead is to throw a new exception that adds more information about what happened e.g.:

public T RestoreObject<T>(string path)
{
    if (!fileSystem.Path.IsPathFullyQualified(path))
            throw new ArgumentException($"Invalid path: '{path}'. It must by fully qualified.", nameof(path));

    try
    {    
        string json = fileSystem.File.ReadAllText($"{path}.json");
        return JsonConvert.DeserializeObject<T>(json);
    }
    catch (Exception ex)
    {
        throw new JsonStorageException($"Could not restore object '{typeof(T).Name}' from '{path}'.", ex);
    }
}

You'll be happy to see the type and file name when something goes wrong.

\$\endgroup\$
  • \$\begingroup\$ Thanks for good tips. But what if ReadAllText throws some IO exception? In your code it will be re-thrown as JsonStorageException and it also doesn't provide details about what went wrong there... \$\endgroup\$ – Glitch Apr 30 at 16:04
  • \$\begingroup\$ @Glitch you pass the original exception as inner-exception. See the last parameter. \$\endgroup\$ – t3chb0t Apr 30 at 16:05
  • \$\begingroup\$ oh, didn't notice that. So you're also saying that I shouldn't have logger instance in my storages, and instead should handle thrown exceptions higher up the stack call? \$\endgroup\$ – Glitch Apr 30 at 16:07
  • \$\begingroup\$ @Glitch exactly, otherwise you have to check the result against null and break the execution there by doing something else. It's easier to use an exception for this especially that you already have one and it doesn't cost anything. \$\endgroup\$ – t3chb0t Apr 30 at 16:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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