thoemmi / TinyLittleMvvm

A small MVVM library for WPF built on top of MahApps.Metro, supporting .NET Framework >= 4.7.2 and .NET Core >= 3
MIT License
131 stars 22 forks source link

Add a Set<T> method to PropertyChangedBase #6

Closed punker76 closed 6 years ago

punker76 commented 6 years ago

This PR does

I like the idea of such method (like in other Mvvm libs), cause it's in most cases a one liner...

Instead

private string _text;

public string Text {
    get { return _text; }
    set {
        if (_text != value) {
            _text = value;
            NotifyOfPropertyChange(() => Text);
        }
    }
}

Now (we can do)

private string _text;

public string Text {
    get { return _text; }
    set { Set(ref _text, value); }
}

or

public string Text {
    get { return _text; }
    set {
        if (Set(ref _text, value)) {
            ValidateAllRules();
        }
    }
}
thoemmi commented 6 years ago

Thanks for your PR. The build breaks not because of your changes but because of a NuGet bug I already reported almost one year ago. See https://github.com/NuGet/Home/issues/4234#issuecomment-276767716 if you're interested.

However, I have to reject your PR for another reason: I don't like the magic of CallerMemberName. I have seen its usage on many MVVM libraries (just like you), but I've never been fond of it. I'll try to justify my decision:

When using CallerMemberName, you have to pay close attention when calling your proposed Set method: in a property's setter, you can omit the propertyName parameter, and the compiler will do its magic. However. when calling the Set method from another location (e.g. a method), you must pass propertyName, otherwise it would pass the name of the calling method. This magic—sometimes you can omit propertyName, sometimes you have to pass it—is an inconsistent user experience... at least in my opinion. TinyLittleMvvm is an opinionated library of mine, therefore I won't add support for CallerMemberName.

But also I'd like to mention that in the last couple of years I hardly ever had to call NotifyOfPropertyChange explicitly. Instead, I use the PropertyChanged add-in for Fody whenever possible, and I never had to implement a backing field again 😉