Otdfbt

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 = @"SomePathConfig";

 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)?

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.