3
\$\begingroup\$

I have a class that is used for serializing and deserializing global application variables. I would like to know if there is anything that I can improve. I know I could inject a dependency to support multiple different serializing formats, but that is not my concern. I would especially want to know if my locking does anything since I am not very proficient in locking.

CurrentLanguage and PointsCount are the data that I serialize.

public static class GlobalSettings
{
    private static CultureInfo _currentLanguage;
    public static CultureInfo CurrentLanguage
    {
        get { lock (Sync) return _currentLanguage; }
        set { lock (Sync) _currentLanguage = value; }
    }

    public static IEnumerable<CultureInfo> AllLanguages => new List<CultureInfo>
    {
        new CultureInfo("en-US"),
        new CultureInfo("pl-PL")
    };

    public static string DbPath => Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Data.db");

    private static int _pointsCount;
    public static int PointsCount
    {
        get { lock (Sync) return _pointsCount; }
        set { lock (Sync) _pointsCount = value; }
    }

    public static bool LoadedSuccessfully = true;
    public static Exception LoadedWithException = null;

    private static bool _isLoaded;
    private static readonly object Sync = new object();
    private static readonly string SettingsFilePath;

    static GlobalSettings()
    {
        var settingsRelativeFilePath = ConfigurationManager.AppSettings["SettingsRelativeFilePath"];
        SettingsFilePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, settingsRelativeFilePath);
    }

    public static void Save()
    {
        lock (Sync)
        {
            if (!_isLoaded)
                throw new InvalidOperationException("Load settings first");

            var serializer = new XmlSerializer(typeof(GlobalSettingsStorage));

            using (var file = File.Create(SettingsFilePath))
            {
                var settings = new GlobalSettingsStorage
                {
                    CurrentLanguageShortName = CurrentLanguage.Name,
                    PointsCount = PointsCount
                };

                serializer.Serialize(file, settings);
            }
        }
    }

    public static bool TryLoad()
    {
        lock (Sync)
        {
            try
            {
                var fileExists = File.Exists(SettingsFilePath);

                var deserializer = new XmlSerializer(typeof(GlobalSettingsStorage));
                using (var reader = File.Open(SettingsFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite))
                {
                    GlobalSettingsStorage settings = null;
                    if (!fileExists)
                    {
                        settings = new GlobalSettingsStorage();
                        deserializer.Serialize(reader, settings);
                    }
                    else
                    {
                        settings = (GlobalSettingsStorage)deserializer.Deserialize(reader);
                    }

                    _currentLanguage = new CultureInfo(settings.CurrentLanguageShortName);
                    _pointsCount = settings.PointsCount;
                }

                _isLoaded = true;
            }
            catch (InvalidOperationException ex) when (ex.InnerException is XmlException)
            {
                LoadedSuccessfully = false;
                LoadedWithException = new SettingsLoadingException("XML settings file corrupted", ex);
                return false;
            }
            catch (UnauthorizedAccessException ex)
            {
                LoadedSuccessfully = false;
                LoadedWithException = new SettingsLoadingException("You do not have access to the settings file.", ex);
                return false;
            }
            catch (Exception ex) when (ex is PathTooLongException || ex is DirectoryNotFoundException || ex is IOException)
            {
                LoadedSuccessfully = false;
                LoadedWithException = new SettingsLoadingException("Failed to load settings file", ex);
                return false;
            }

            return true;
        }
    }
}

[Serializable]
public sealed class GlobalSettingsStorage
{
    public string CurrentLanguageShortName { get; set; }
    public int PointsCount { get; set; }

    public GlobalSettingsStorage()
    {
        CurrentLanguageShortName = ConfigurationManager.AppSettings["DefaultLanguage"];
        PointsCount = int.Parse(ConfigurationManager.AppSettings["DefaultPointsCount"]);
    }
}

I have property LoadedSuccessfully, because in WPF I want to show a user information if settings were not properly loaded and I show it as a popup.

public async void OnDialogHostLoaded()
{
    if (!GlobalSettings.LoadedSuccessfully)
    {
        var ex = GlobalSettings.LoadedWithException;
        var message = string.Join(": ", ex.Message, ex.InnerException?.Message);
        var popup = new PopupBoxView
        {
            DataContext = new PopupBoxViewModel(PopupBoxType.Ok, message, true)
        };

        NLog.LogManager.GetCurrentClassLogger().Fatal(ex);
        await DialogHost.Show(popup, "RootHost");
        Application.Current.Shutdown();
        return;
    }
}
\$\endgroup\$
2
\$\begingroup\$
public static int PointsCount
{
    get { lock (Sync) return _pointsCount; }
    set { lock (Sync) _pointsCount = value; }
}

I wonder why this property has a public setter? Is there a scenario where setting this form the outside would make sense?


Other things...

I like your exceptions. They clearly tell me what went wrong so I would instantly know how to fix that. You even have created a custom one SettingsLoadingException, kudos!

I would however change how you handle this:

public static bool LoadedSuccessfully = true;

Make it false by default and set it to true only once when TryLoad succeeded. Currently you set it three time to false instead.

You could also flip the logic with return true and do it onyl once before the first catch, this is, inside the try and do return false only once at the end of the method.

\$\endgroup\$
1
\$\begingroup\$

You don't show how you actually call TryLoad(). So with that in mind here are some comments:

public static bool LoadedSuccessfully = true;
public static Exception LoadedWithException = null;

private static bool _isLoaded;

What is the difference between LoadedSuccessfully and _isLoaded? The one is initially set to true, while the other is set to false, but they both indicate if the load was successful or not. I think you can remove one of them.

Another issue is that LoadedSuccessfully and LoadedWithException are public settable. If they are of any use - see next comment - I would change them to private settable properties instead.

These three fields have only meaning for the state of the object in the load process, because - as I read OnDialogHostLoaded() - you close the application in a following step in the load process, if loading the settings went wrong. In other words: you probably don't have a running application with GlobalSettings.LoadedSuccessfully = false. I think, I would remove these fields and reconsider the load process so you close the application immediately if TryLoad() fails like:

void OnLoad() // Or what ever you call it
{
  try
  {
    GlobalSettings.Load();
  }
  catch (SettingsLoadingException ex)
  {
    // TODO: log and exit application
    // Maybe fire an event to let the main app notify user via UI
  }
  catch (Exception ex)
  {
    // TODO: log and exit application
    // Maybe fire an event to let the main app notify user via UI
  }
}

Here TryLoad() is renamed to just Load() because it doesn't return a boolean but only throws on failure:

public static void Load()
{
  lock (Sync)
  {
    try
    {
      var fileExists = File.Exists(SettingsFilePath);

      var deserializer = new XmlSerializer(typeof(GlobalSettingsStorage));
      using (var reader = File.Open(SettingsFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite))
      {
        GlobalSettingsStorage settings = null;
        if (!fileExists)
        {
          settings = new GlobalSettingsStorage();
          deserializer.Serialize(reader, settings);
        }
        else
        {
          settings = (GlobalSettingsStorage)deserializer.Deserialize(reader);
        }

        _currentLanguage = new CultureInfo(settings.CurrentLanguageShortName);
        _pointsCount = settings.PointsCount;
      }
    }
    catch (InvalidOperationException ex) when (ex.InnerException is XmlException)
    {
      throw new SettingsLoadingException("XML settings file corrupted", ex);
    }
    catch (UnauthorizedAccessException ex)
    {
      throw new SettingsLoadingException("You do not have access to the settings file.", ex);
    }
    catch (Exception ex) when (ex is PathTooLongException || ex is DirectoryNotFoundException || ex is IOException)
    {
      throw new SettingsLoadingException("Failed to load settings file", ex);
    }
    catch (Exception ex)
    {
      throw new SettingsLoadingException("General exception occured", ex);
    }
  }
}

I've added a general catch clause to catch all unknown exceptions by, so that you can rethrow a SettingsLoadeingException for them as well.


I imagine that the reason for having LoadedSuccessfylly is that you postpone handling a possible failure in GlobalSettings.TryLoad() to OnDialogHostLoaded(). IMO you handle the settings failure in the wrong place: what have loading the settings to do with loading the "dialog host"?


You only catch specific exceptions. What happens to more general exceptions or special exceptions that don't meet the conditions in your catch clauses?


        GlobalSettingsStorage settings = null;
        if (!fileExists)
        {
          settings = new GlobalSettingsStorage();
          deserializer.Serialize(reader, settings);
        }
        else

Why is it necessary to save an empty settings file, if it doesn't exist when trying to load it? You somehow break the single responsibility principle here.

\$\endgroup\$

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.