Calling a private method in a setter to update object at every change of the property

Question

Code below shows setting a value of an object's property and calling a private method in a setter to update the status of the object. Is this call a good practice or setter at most should only validate incoming value and should not take part in other kinds of logic?

public class Toolbar
{
    private bool isMenuButtonVisible;
    public bool IsMenuButtonVisible
    {
        get => isMenuButtonVisible;
        set
        {
            isMenuButtonVisible = value;
            UpdateToolbarVisualState();
        }
    }

    private void UpdateToolbarVisualState()
    {
        MenuButtonBackground.IsVisible = IsMenuButtonVisible;
        MenuButtonIcon.IsVisible = IsMenuButtonVisible;
        MenuButtonLabel.IsVisible = IsMenuButtonVisible;
        //...
    }
}

Which UI Framework are you using? WPF? And what is Toolbar representing? The control/view itself? — helb, Feb 13 '20 at 13:01
INotifyPropertyChanged is usually implemented this way, I wouldn't say its generally bad practice but you have to be careful about side-effects. — helb, Feb 13 '20 at 13:17
I am using Xamarin.Forms. Toolbar is representing a view/layout which contains children. — apex39, Feb 13 '20 at 13:23

Ryathal · Accepted Answer · 2020-02-13T15:25:13.227

Calling methods in getters and setters is a dangerous practice. It can be acceptable for minor things in some cases, your example is a good one of an acceptable case. The reason it's a dangerous practice is because it exposes object state outside the object and moves the reasoning of that state outside the object as well. For extremely simple cases it can be easier to just have a method called as part of the process, but this will create more potential side effects and increase the difficulty of using your object. A better solution would be separate Hide() and Show() methods rather than setting a property on the object.

The reason explicit methods would be preferred is that you are now telling the object what to do rather than asking. In the case of visibility it probably doesn't change much, but it's good to get in the habit of following this pattern for more complicated procedures. As an example if you were to add paging functionality, you don't want to ask the object what page its on, increment the result and tell it to go to that page. You just want to tell your object go to the next page, let the object itself handle calculating the next page, handling if there isn't a next page, or anything else that might be needed. The proper object oriented approach is to keep the state of an object encapsulated as possible and only expose methods that are things that the object should do.

By "your example is a good one" you mean that my example is acceptable? — apex39, Feb 13 '20 at 14:14

Calling a private method in a setter to update object at every change of the property

1 Answers1