Should a setter return immediately if assigned the same value?

Question

In classes that implement INotifyPropertyChanged I often see this pattern :

    public string FirstName
    {
        get { return _customer.FirstName; }
        set
        {
            if (value == _customer.FirstName)
                return;

            _customer.FirstName = value;

            base.OnPropertyChanged("FirstName");
        }
    }

Precisely the lines

            if (value == _customer.FirstName)
                return;

are bothering me. I've often did this but I am not that sure it's needed nor good. After all if a caller assigns the very same value I don't want to reassign the field and, especially, notify my subscribers that the property has changed when, semantically it didn't.

Except saving some CPU/RAM/etc by freeing the UI from updating something that will probably look the same on the screen/whatever_medium what do we obtain?

Could some people force a refresh by reassigning the same value on a property (NOT THAT THIS WOULD BE A GOOD PRACTICE HOWEVER)?

1. Should we do it or shouldn't we?

2. Why?

Fedora

Answer 1

Yes, you should return immediately when the consumer is setting a property value that is equal to the value which is already being persisted.

First of all, there is no reason to waste any time or resources in the setter of the property - the value is already set so no further action is needed. Also you should never call OnPropertyChanged if the value stored in the property's backing field hasn't changed - the method is intended to be raised when the value has changed not when the property's setter has been called.

All that being said, however - if the setter didn't have a call to OnPropertyChanged I wouldn't have bothered to check the value first. In the case of a simple setter that only sets the backing field's value and nothing else it is actually going to be faster to always the set the value rather than checking first then setting the value. Only use this pattern when the property's setter has additional logic that either shouldn't fire or may incur an unnecessary performance penalty.

Hear

I think I would disagree that your #2 is true. Depending on what is being done. Comparing two strings is much more expensive than settings the reference to a string. Also you have added processing always, even if they are different. – Overtax

To add to my previous statement, calling OnPropertyChanged may still be desired depending on the intent. There may be times that even if the value did not change, you may want to enter the OnPropertyChanged event just because the value had been entered. – Overtax

I disagree with #1 point. What you call waste of resouces could lead to a missing notification. Value is changed when setter is called. It could be the same value, but it was changed to same value. – Adne

Answer 2

Or you could do this:

   set
    {
        if (value != _customer.FirstName)
       {

           _customer.FirstName = value;

          base.OnPropertyChanged("FirstName");
       }
    }

No need for multiple return paths.

To further answer your question, I wouldn't force an update to property if it's being overwritten by the same value. There's really no point, because you're probably not going to get any benefit from it. (I could see an instance where you would want to track each time someone tries to update a value.)

Nine

Anyway I think that what he is asking is not about the exact code path, but rather if it's valid to avoid executing the setter code on duplicate settings. (Work like PUT and not like POST for you web dev guys.) – Rosalvarosalyn

No need to increase nesting either. :) – Detribalize

Actually, a lot of people think that you shouldn't ommit the {} on an if statement so: if(..){return;}. Still nesting, and you have multiple return paths. – Nine

As a personal preference, I don't like adding curly brackets for one-statement blocks... – Nathanialnathaniel

@Andrei: the reasoning behind always using the braces is to minimize the possibility of error. I don't like them either, so I moved on to Python :) – Rosalvarosalyn

Answer 3

The only argument against that pattern (where you return if the value hasn't changed) I can think of is the purist's view that every function should have only one exit. Not being a purist, I don't agree. I see nothing wrong with breaking out if the value hasn't changed, avoiding the notification update.

Frediafredie

I've never been convinced by the one exit path to every function/method point. – Rosalvarosalyn

I've personally come across too many scenarios where it's not viable to limit every function to a single exit path without unnecessarily splitting it into multiple functions, and even then it's not always avoidable without obfuscating your code (if more than half of the function is inside of an else block, IMHO you're doing it wrong). – Publishing

Answer 4

The only situation when you shouldn't use it is when you know that you can have dirty data on your class, for example on a ORM layer object that might have outdated values compared to the underlying database because of modification by another user.

If that situation doesn't affect you, then cache away!

---

_Edit

I misunderstood your question, as you are talking about setters, not getters.

Similar points apply. If the set operation is a costly one, and is not supposed to have any kind of side effect (it should't! Side effects on setters are <blink>evil</blink>!!1), then it's a valid optimization.

Tram

Answer 5

One reason to not return early, I would imagine, is for subscribers that joined the party late. They might not be aware of the object's current state, and will miss out on the setter notification if you return early.

Krigsman

Either the late joiners fed on the current value when they joined, or it wasn't relevant: if they did, they don't need to be updated; if they didn't, they opted to ignore it and don't need to be notified of a change that didn't happen. Either way raising the event is irrelevant. – Detribalize

@sr, you are laying emphasis on the value. I am giving more importance to the event that triggered the value (not that the value is unimportant, but it's secondary to the event itself). there are perfectly valid use cases where some other object only needs to know the value as it happened, and is not necessarily interested in knowing what happened before it subscribed. if you cannot think of such a use-case, let me know, and i'll give you several examples. – Krigsman

I appreciate that you made the point, which is very relevant to certain applications. I recently had to revamp an aspect oriented solution for applying property change notifications because the aspect was always immediately returning if the value was equal. That's what I want most of the time, but I ran into a use case for being interested in the event itself, not the actual value. Now I have aspects which can do either. I can see an argument for having a separate event to observe rather that an "on change" event, though. – Loquacious

Answer 6

Most of times those properties are used in binding. But as soon as you start using NotifyPropertyChanged event yourself (in own markup extenstions, behaviors, between VM in MVVM, etc.) you will realise you just want this event to always occurs.

To me this check (notification protection) is kind of premature optimization. I am often rising event with "" as propertyName to just refresh bindings or force event for some custom stuff and that alone is much more costly.

I don't see a need to protect each property. Why? From what kind of operation? Animations are running on dependency properties. User updates (when view bindings are updating source) are anyway slow. And for any update rised from within code behind you are pretty much need event to be rised.

To me it looks like a pattern, invented without much reasons and followed blindly. Of course there are cases when you need to prevent property setter code from being running under certain conditions. And that's ok if you add such checks to solve certain performance issue then. But not in advance.

Adne