1

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;
        //...
    }
}
New contributor
apex39 is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
  • Which UI Framework are you using? WPF? And what is Toolbar representing? The control/view itself? – helb Feb 13 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 at 13:17
  • I am using Xamarin.Forms. Toolbar is representing a view/layout which contains children. – apex39 Feb 13 at 13:23
1

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 at 14:14
  • @apex39 yes, updated to be a bit more clear. – Ryathal Feb 13 at 15:25

Your Answer

apex39 is a new contributor. Be nice, and check out our Code of Conduct.

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.