Should I have entities that automatically persist?

Question

In a MVVM application (Xamarin.Forms, FWIW) I have a viewmodel that stores settings explicitly

public class SettingsPageViewMode : INavigatedAware
{
    ISettingsRepository settingsRepository; // injected

    public void OnNavigatedTo(NavigationParameters navigationParameters)
    {
        var settings = settingsRepository.LoadSettings();
        this.SomeBoolSetting = settings.SomeBoolSetting;
    }

    public bool SomeBoolSetting
    {
        get => someBoolSetting;
        set
        {
            if(someBoolSetting == value)
            {
                return;
            }

            someBoolSetting = value;
            OnPropertyChanged("SomeBoolSetting");
            UpdateSettings();
        }
    }

    private void UpdateSettings()
    {
        var settings = settingsRepository.LoadSettings();
        settings.SomeBoolSetting = SomeBoolSetting;
        settingsRepository.SaveSettings(settings);
    }
}

I am thinking about if it's a good idea to have settings store itself automatically via a decorator that is instantiated when ISettingsRepository returns a Settings object

internal class AutoPersistSettingsDecorator : Settings
{
    Settings settings;
    ISettingsRepository settingsRepository;

    public AutoPersistSettingsDecorator(Settings settings, ISettingsRepository settingsRepository)
    {
        this.settings = settings;
        this.settingsRepository = settingsRepository;
    }

    public override bool SomeBoolSetting
    {
        get => settings.SomeBoolSetting;
        set
        {
            if(settings.SomeBoolSetting == value)
            {
                return;
            }

            settings.SomeBoolSetting = value;
            settingsRepository.SaveSettings(settings);
        }
    }
}

This would simplify the viewmodel and make saving the settings transparent

public class SettingsPageViewMode : INavigatedAware
{
    ISettingsRepository settingsRepository; // injected
    Settings settings;

    public void OnNavigatedTo(NavigationParameters navigationParameters)
    {
        var settings = settingsRepository.LoadSettings();
        this.SomeBoolSetting = settings.SomeBoolSetting;
    }

    public bool SomeBoolSetting
    {
        get => settings.SomeBoolSetting;
        set
        {
            if(settings.SomeBoolSetting == value)
            {
                return;
            }

            settings.SomeBoolSetting = value;
            OnPropertyChanged("SomeBoolSetting");
        }
    }
}

Possible drawbacks:

• Settings looks like a data type, but comes with a behavior
• It's not really obvious that Settings is persisted
• Might violate the principle of least astonishment
• When the public interface of Settings changes, AutoPersistSettingsDecorator has to change, too
• Might be error prone

However, Clean Code still proposes (more or less) this design (I don't have a page number, since I'm reading on a Kindle, but it's in part 11 (Systems), in the sections about AOP, just after Figure 11-3)

The client believes it is invoking getAccounts() on a Bank object, but it is actually talking to a set of nested decorator objects that extend the basic behavior of the Bank POJO.

Did I get something fundamentally wrong? Should I create the settings class more use-case centred? How could I possibly achieve this, since I basically have a very data-like view of a settings class. Is this approach appropriate at all when it comes to settings?

Answer 1

I see no problem behind idea of having self-persisting settings.

But I really don't like implementation. First thing, I would create ISetting<T> interface, with T Value property. Then, instead of bool SomeBoolSetting I would have ISetting<bool> SomeBoolSetting and bind to SomeBoolSetting.Value. This way, you can have lazy-loading of your settings, instead of having to manually load in in OnNavigated and have automatic persistence in the base class instead of calling UpdateSetting in every property in view model. This also simplifies the possible decorators, as you only need one type of decorator.

Quick hack what I mean. It assumes ISettingRepository can save and load based on key instead of having properties to be set:

  public interface ISetting<T> : INotifyPropertyChanged
  {
     T Value { get; set; }
  }

  public interface ISettingsRepository
  {
     void SaveSetting(string name, object value);

     object LoadSettings(string name);
  }

  public class PersistedSetting<T> : ISetting<T>
  {
     private T _value;
     bool hasValue;
     private readonly ISettingsRepository _repository;
     private readonly string _key;

     public PersistedSetting(ISettingsRepository repository, string key)
     {
        _repository = repository;
        _key = key;
     }

     public T Value
     {
        get
        {
           if (!hasValue)
           { 
              // lazy loaded
              _value = (T)_repository.LoadSettings(_key);
           }

           return _value;
        }
        set
        {
           if (hasValue && _value.Equals(value))
           {
              return; // dont update if same
           }

           _value = value;
           _repository.SaveSetting(_key, _value); // persisted every time it changed
           hasValue = true;

           PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value)));
        }
     }

     public event PropertyChangedEventHandler PropertyChanged;
  }

  public class SettingsPageViewMode
  {
     ISettingsRepository settingsRepository;

     // seriously here. Use constructor injection
     public SettingsPageViewMode(ISettingsRepository settingsRepository)
     {
        this.settingsRepository = settingsRepository;

        SomeBoolSetting = new PersistedSetting<bool>(settingsRepository, "SomeBool");
     }

     public ISetting<bool> SomeBoolSetting { get; } // bind to SomeBoolSetting.Value
  }