1
\$\begingroup\$

The following code works, but is it a good idea to call another method in a setter?

public TypeSignalDTO SelectedTypeSignal
{
    get { return _selectedTypeSignal; }
    set
    {
        _selectedTypeSignal = value;
        RaisePropertyChanged(() => SelectedTypeSignal);
        UpdateExtremitiesLosses(); //This really bugs me.
    }
}


public void UpdateExtremitiesLosses()
{
    if (_completeLink.Any() && SelectedTypeSignal != null)
    {
        try
        {
            _lossService.SetLinkLosses(SelectedTypeSignal.Id_TSI, _completeLink);
            ShowLossColumn = true;
            TotalLoss = _completeLink.Last().LossAtCurrentPoint;
        }
        catch (ServiceException ex)
        {
            ShowLossColumn = false;
            TotalLoss = 0;
            _dialogService.ShowErrorMessage(ex.Message);
        }
        finally
        {
            Extremities = _completeLink.ToViewModel().ToList();
        }

    }
}

As you can see the setter calls another method: UpdateExtremitiesLosses which in turn changes other properties on the view model ShowLossColumn, Extremities and TotalLoss.

I know that properties are actually functions, but changing another property when calling a setter seems like an unintended side effect to me.

Being a beginner with WPF this really seems wrong to me. I would like to know if it is a better idea to subscribe to the ChangeEvent and call the method there, or just call the method in the setter.

\$\endgroup\$
4
\$\begingroup\$

The access to a property should be fast. If anything in a getter or setter of a property needs more time than usual to return it should better be a method.

So if UpdateExtremitiesLosses() is rather time consuming or if it should react on the changes made and there is an event which should handle changes like you mentioned, this call should better live inside this event.

public TypeSignalDTO SelectedTypeSignal
{
    get { return _selectedTypeSignal; }
    set
    {
        _selectedTypeSignal = value;
        RaisePropertyChanged(() => SelectedTypeSignal);
        UpdateExtremitiesLosses(); //This really bugs me.
    }
}  

If you haven't done it already, you should override the =, != operators together with the Equals() and GetHashCode() methods and check before you call RaisePropertyChanged() if they are equal by any chance. If they are equal you shouldn't raise the event to prevent unneeded code execution.

RaisePropertyChanged(() => SelectedTypeSignal);  

here you should simply use the _selectedTypeSignal instead of the property, because there is no need to call the getter of the property if you already has its value stored in a local variable.

\$\endgroup\$
  • \$\begingroup\$ If TypeSignalDTO is a class, I wouldn't advise overriding the == and != operators. \$\endgroup\$ – Willem van Rumpt Oct 12 '15 at 7:03
  • \$\begingroup\$ The event i was talking about is the PropertyChanged, does your response still apply in this case because if it does it would contradict the other answer from @WillemvanRumpt or maybe i just didn't get it. As for the check i already did it just removed it from the code. My problem is basically calling a function that changes the state of the object in the setter seams like an unexpected side effect \$\endgroup\$ – Bobby Tables Oct 12 '15 at 12:06
  • \$\begingroup\$ That event is, like Willem van Rumpt pointed out, for others to subscribe. This don't mean that the same class couldn't use it. If a centralized place is needed for processing changes then that is IMO the way to go. \$\endgroup\$ – Heslacher Oct 12 '15 at 12:11
  • \$\begingroup\$ @toby: My wording was a bit too strict. It's not that you can't, or definitely shouldn't do it, it's just that it's not the main purpose of the event, and, IMHO, you should try to avoid (in general) consuming your own published events, unless you have a very good reason to do so. As for side effects: A setter affecting more than one part of the internal state of an object is not a problem. Just make sure the internal state is in order before you notify possible third parties and/or leave the setter, so nobody can "see" an object in a state it's not supposed or expected to ever be in. \$\endgroup\$ – Willem van Rumpt Oct 12 '15 at 14:53
  • \$\begingroup\$ @toby: And yes, as holroy points out, a setter updating other public settable properties (..updating other settable properties...) is indeed to be avoided. \$\endgroup\$ – Willem van Rumpt Oct 12 '15 at 15:08
4
\$\begingroup\$

In your update you state that UpdateExtremitiesLosses also updates other properties, in other words you are causing a ripple effect if updating this property. That is bad!

The best way to handle this, in my opinion, is to use some variation of letting dependent properties subscribe to change notifications. This could be the same as the externals get access to, or you could make an internal variant in addition.

Another way of handling it is to reconsider your properties, as properties are commonly supposed to be independent of each other, or a calculated version of other properties. That is you might need to change the getter of the other property thusly removing the need of UpdateExtremitiesLosses as it now is a calculated property, or you could remove the dependency by refactoring your properties.

In essence, No, it is not good to call methods which has side effects unrelated to the specific property of your setter!

\$\endgroup\$
  • \$\begingroup\$ I suggest changing the wording of "No, it is not good to call another method in your setter!". In the OP's scenario, it's definitely true. But to me the sentence reads as an overly broad (and false advise) "do not call methods from your setter". \$\endgroup\$ – Willem van Rumpt Oct 12 '15 at 15:11
  • \$\begingroup\$ I'm struggling to find a wording that actually describes the scenario clearly, as the properties his method update clearly are related to the current property, but it's a definite improvement. That, together with your first paragraph, definitely highlights the unwanted scenarios for me at least, one up :) \$\endgroup\$ – Willem van Rumpt Oct 12 '15 at 15:21
1
\$\begingroup\$

Assuming UpdateExtremitiesLosses() performs part of maintaining internal state after a change of the SelectedTypeSignal, this is perfectly fine, although in that case, you may want to call it before notifying subscribers.

The PropertyChanged event of INotifyPropertyChanged is there for interested third-parties to subscribe to and monitor property changes, it's not for use by the implementing class.

\$\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.