CODEWITHSUNDEEP
c#inotifypropertychanged
Matt H
Matt H

Reputation: 7389

When a property is set, should its value be updated even if it did not change?

This can be applied to many different languages, but I am specifically working with C# and INotifyPropertyChanged.

If a property is set, should I re-assign its value even if it did not change? For example:

public int IntProperty
...
set
{
    var oldValue = _intProperty;
    _intProperty = value;
    if (!Equals(value, oldValue))
    {
        OnPropertyChanged(...);
    }
}

vs.

public int IntProperty
...
set
{
    if (!Equals(value, _intProperty))
    {
        _intProperty = value;
        OnPropertyChanged(...);
    }
}

I just can't decide. Technically, it shouldn't make a difference, but there are some odd corner cases (like strange Equals implementations that should never happen) that could change the behavior.

Upvotes: 2

Views: 2377

Answers (4)

Servy
Servy

Reputation: 203802

This can be influenced by the type of the property. Some classes have intelligent Equals methods, and some don't. If you trust yours, it shouldn't matter, but if the class doesn't override Equals, or doesn't do it well, then it changes things. On the whole, I'd say that the underlying value shouldn't change without the notification being sent, so I'd go with #2.

To give an example, maybe there's a simple data holder looking something like this:

class Data
{
    public int ID { get; set; }
    public string SomeData { get; set; }
    public string SomeOtherData { get; set; }
    //Assume there are lots of other fields here
    public override bool Equals(object obj)
    {
        Data other = obj as Data;
        if (other != null)
        {
            return ID == other.ID;
        }
        return false;
    }
}

Now if ID is supposed to be unique (possibly because it maps to a DB ID value) then this isn't that bad of an idea, but it doesn't stop people from going around creating objects with the same ID and different data values. If some mean person did that you could end up setting a value to a property such as your in which the new value really is significantly different, but .Equals will say they are the same. Now no matter what happens here you're somewhat screwed, but which is worse, not setting a value that shouldn't be different but is (because the set is inside the if) or changing the value and not notifying event subscribers that the event changed? (You also have the third option of setting the event and notifying people even if it changed, possibly indicated that this is the case via the event args.) If an event subscriber is code that persists the change to the database (which is often the case with this type of structure) then you're changing a value in memory, thinking that it really changed (getting it back out is an exact reference match), but the value still won't be persisted because the event wasn't fired.

Sorry for the wall of text.

tl;dr version, users of this code can find a way to screw you no matter what you do; eventually you need a minimal amount of trust in them for your code to be effective.

Upvotes: 2

Wonko the Sane
Wonko the Sane

Reputation: 10813

Short answer - generally, either check if they are different and set it only if it is, or don't bother checking.

Longer answer - For MVVM, one accepted practice is to abstract the set to a base class, where you have two overloads of a function to set the property. This should handle all cases of setting the property, makes them easier to implement in your inherited classes, and cleans things up.

public event PropertyChangedEventHandler PropertyChanged;

public Boolean SetProperty<T>(string propertyName, ref T field, T value, IEqualityComparer<T> comparer)
{
    if (!comparer.Equals(field, value))
    {
        T oldValue = field;
        field = value;

        if (PropertyChanged != null)
            PropertyChanged(this, new PropertyChangedEventArgs(propertyName));

        return true;
    }

    return false;
}

public Boolean SetProperty<T>(string propertyName, ref T field, T value)
{
    return SetProperty(propertyName, ref field, value, EqualityComparer<T>.Default);
}

Then, in your ViewModel, you would do something like this:

private int mMyIntProperty = 0;
public string MyIntProperty
{
    get { return mMyIntProperty; }
    set { SetProperty("MyIntProperty", ref mMyIntProperty, value); }
}

For simple types, you don't need to specify a Comparer, as the default will do just fine. However, having the comparer available makes checking complex types easier.

Upvotes: 1

M.Babcock
M.Babcock

Reputation: 18965

If you're going to do the check either way then I'd say not to worry about setting the value if they are equal. Save yourself the cycle.

Upvotes: 1

Michel Keijzers
Michel Keijzers

Reputation: 15347

It has no use setting it again, it only takes time/saves performance.

And since already the notification is inside the if statement, it also is not more code.

Upvotes: 1

Related Questions