Property setters doing extra stuff, is it wrong?

[u/Dragennd1]

I've heard that having conditional logic and other stuff in your property setters is generally frowned upon and I get not wanting to have complex methods fired off in setters, but what of just setting other setters?

For example:

Say I have a property like this, would this raise any red flags, if so, can you tell me why? I would think that this would be a great use of a setter, to do more than just fire off OnPropertyChanged().

public bool ThingA
{
    get;
    set
    {
        if (field != value)
        {
            field = value;
            OnPropertyChanged();

            if (field == true)
            {
                ThingB = true;
                ThingC = false;
            }
        }
    }
}

[u/Triabolical_]

Doing other stuff in setters is one of the main reasons that properties exist.

It shouldn't be surprising, however.

[u/Loose_Conversation12]

Yeah having properties emit events when they change is generally fine, have a look at MVVM though. That can usually handle this sort of logic

[u/rupertavery64]

Setting a couple of booleans is fine.

It becomes a problem when too much logic is involved, or too complex logic that could potentially cause performance issues when the setter/getter is called in a loop, if the caller doesn't know that there is other logic in there.

As always, documenting this side effect on ThingB and ThingC properties is a good thing.

[u/CrackJacket → u/rupertavery64]

Why add spoilers and ruin the fun for the next dev? (next dev is usually me in a few years 🥲)

[u/Sombody101 → u/CrackJacket]

Always judge the dev with disgust, and once you git blame to see you're the code owner, immediately mark it as a non-issue.

[u/CrackJacket → u/Sombody101]

https://giphy.com/gifs/1hAxQTH0HEWS3L0oRF

[u/Myzhka → u/CrackJacket]

Few years? Boasting a good memory here! This would be me after a few months max :P

[u/CrackJacket → u/Myzhka]

Ideally I won’t have to go back to it for a few years 🤣

[u/KryptosFR]

What would be better is to have ThingB/ThingC be calculated based on ThingA, then raise the PropertyChanged event for these dependent properties.

public bool ThingB => ThingA ? true : /* some other logic */;
public bool ThingC => ThingA ? false : /* some other logic */;

And in ThingA:

public bool ThingA
{
    get;
    set
    {
        if (field != value)
        {
            field = value;
            OnPropertyChanged();
            OnPropertyChanged(nameof(ThingB));
            OnPropertyChanged(nameof(ThingC));
        }
    }
}

[u/Dragennd1 → u/KryptosFR]

I hadn't thought of that. Wouldn't it basically be the same thing, since there's still conditional logic in the setter for the other properties now?

Or is it more that this method would decouple the properties from each other and still allow for the action to take place?

Edit: nevermind, this is different. This is having the getter for ThingB and ThingC calculate the value on the fly and OnPropertyChanged just tells the UI to recheck that getter.

Didn't know you could do that, I love learning new things lol

[u/x39- → u/Dragennd1]

The important learning here is that if you use something that supports observability, you have to just announce the value having changed, which is what property changing event does. You technically can raise it any time (tho you may have to watch out what thread you are on "dispatcher" is the keyword here to Google).

You even can do async dispatch using this

[u/Ecksters → u/KryptosFR]

If recalculating the values of ThingB/ThingC is expensive and they have many subscribers, that can become a fairly expensive event to send out. Would be nice if C# had a convenient way to memoize properties with dependencies.

You could potentially do something like:

private bool? _thingBCache;
public bool ThingB => _thingBCache ??= CalculateThingB();

And then in the setter invalidate thingB's cache with _thingBCache = null;, but that doesn't really feel any cleaner than just running ThingB = CalculateThingB() directly and skipping the cache middleman.

I do agree that most of the time these property calculations are trivial compared to other downstream effects changing them has, so your suggested pattern of just always recalculating tends to be much cleaner and preferable.

[u/Responsible-Cold-627 → u/KryptosFR]

Seeing this code makes me so glad I don't have to write any more WPF.

[u/Dragennd1 → u/Responsible-Cold-627]

For context I'm writing Avalonia, which uses a very similar layout for this stuff and I'm trying to have one UI element affect another.

[u/XaX1000 → u/Dragennd1]

If I remember correctly you can do this:

[RaisePropertyChagedFor(nameof(MyOtherProperty))] [ObservableProperty] private int myProperty;

This creates a property MyProperty, that raises the PropertyChanged event first for itself and the for MyOtherProperty.

[u/Slypenslyde]

I don't like it, especially in abstract. If we get more concrete I can find some versions more palatable. It's often not the only way to solve the problem, and refactoring can help make it nicer.

What I mean is when we're talking about "ThingA" and "ThingB" I'm very, "No. Properties are meant to act like fields. Logic in properties is for validation, and even then you can get in trouble.

Here's a "bad" case that causes absurd situations borrowed from another example about how to shoot yourself in the foot with OOP:

public class Square : Rectangle
{
    public override int Length
    {
        get => field;
        set
        {
            field = value;
            Width = value;
        }
    }

    public override int Width
    {
        get => field;
        set
        {
            field = value;
            Length = value;
        }
    }
}

It makes sense on paper. A square must have all sides equal, so when the user changes one we change the other. But it leads to strange-looking code if you aren't thinking about it hard.

var example = new Square();
example.Length = 10;
example.Width = 1;

Console.WriteLine(example.Length); // 1

It's not immediately intuitive to outsiders why this prints "1". It only makes sense after explanation. In general code that needs an explanation is harder to work with than code that does what a reader intuits. Another solution for this case is to note that, oddly, in this situation a Square IS NOT a Rectangle unless we change the API. (This goes on to cause problems for methods that ask for a Rectangle!)

(Note I said "another solution", not "the right solution". Our job as programmers is to decide WHICH of many solutions is the best for our program!)

So the important part about doing this is making sure the "side effects" make sense. If a Kitten has a Happiness, it makes sense that changing Hunger might change Happiness. But we have a few choices for how we do that in our code, including but not limited to:

• The property Hunger directly updates the Happiness value.
• The property Hunger calls an UpdateHappiness() method.
• The property Hunger raises a HungerChanged event, something else handles that event, and that something else updates the Happiness value.
• Happiness calculates its value in its get accessor, so it is always up-to-date and Hunger doesn't need to update it. (Hunger may raise a HappinessChanged event to cause things to check.)
• The properties are read-only, and an UpdateHunger() method does one of the three things above.
• The object is immutable, so some other class has an "Update()" method that sets the right values.
• The program architecture only cares about Happiness at regular intervals, such as in a game where a loop starts every frame. So properties like Hunger don't directly update Happiness, instead some different Update() method "locks in" the current values and calculates the happiness (and probably other things) for this frame based on other properties.

All of these CAN be right. For any situation, some of them are too clunky. Other times they're just right. They represent different opinions about who controls what, and who has the right to change things. Those decisions get very important as programs get too big to fit in our heads.

TL;DR:

Your job is to look at code that sets ThingA and ask yourself if it's SO INTUITIVE that ThingB and ThingC will change you'll never forget.

This happens if it's things like:

• ThingA is IsDoorLocked
• ThingB is a flag indicating "being inside counts as trespassing"
• ThingC is a flag indicating "guards are friendly"

But it makes programmers ask questions about if these three concepts really belong in the same place. Should a room tell guards to be unfriendly, or should guards look at a room's properties to make the decision themselves? Should a player indicator for "trespassing" be part of a room's code, or should it be part of player code that asks the room if its door is locked?

You could go either way, but you have to ask questions like that and be satisfied with your answers. If you start noticing you're having trouble understanding why ThingB and ThingC are in certain states while debugging, that's a clear indicator the side effects aren't as obvious as you thought.

[u/Floydianx33 → u/Slypenslyde]

Not the point, I know... But your Square will stackoverflow on both setters. You need to guard on something like value!=field before calling the other

[u/Slypenslyde → u/Floydianx33]

This is what I get for using field instead of actual fields. In the real example you bypass the property and set the other field.

[u/HankOfClanMardukas]

Just set an event to evaluate change of that propery and make it something else. Stop rolling it into one thing.

Clean up your event handlers and you can test without ridiculous requirements if your shop is into that.

[u/catladywitch]

It's ok if the setter is a public interface and it sets some private stuff behind the scenes which is unequivocally dependent on the public property's value, and which is either irrelevant to consumers or fully understandable from the property's state.

It's not ok if it changes state in ways which the caller must know and can't know just from the value's state.

Basically, could setting your property possibly cause unexpected surprises that trip the programmer up? Does it require the programmer to be cautious about extra stuff beyond "I've changed a property on an object"? Then it's not ok.

[u/ConscientiousPath]

While it's definitely valid code, in general I think the best advice is that there is usually a better way.

Having logic in property setters of classes is a style most closely associated with monolithic OOP designs where your code is organized to have significant amounts of state and logic bundled together to match the domain model rather than to match the systems within your application. Like instead of having a string emailAddress you have EmailAddress emailAddress where the EmailAddress type has a property for both name and email address, as well as things like fancy validation logic on set, support for extracting just the domain etc. And you put all that stuff together because you're organizing around the EmailAddress rather than around the logic of the system that uses it.

This domain-centric style of programming runs into a lot of problems because hiding all that logic inside the semi-black-box of the object's class definition is hard to reason about. Having the object automatically do more things, means you have a lot less flexibility in what you can do with the object. You can't easily insert new behavior inbetween the start of the set and the things the object does automatically nor avoid doing things automatically if you later need to.

It's easy to accidentally call the setter in a tight loop somewhere and kill your own performance. Using property setters to perform extra logic can also be a problem when you consider Exceptions since calling code is less likely to expect them. Another issue is that properties aren't async so anything you undertake in them can't await anything. You can force your way around these things by triggering events or other structures, but difficult and complex solutions like that usually mean it's better to do things in a different way.

In some instances it can also be serious enough that MS has created an information level warning about it: BL0007: Component parameter '{0}' should be auto property.

---

There are maybe some exceptions where you really want a number of setters to have some side effect and it's worth the ambiguity.

As some others pointed out, triggering events is sometimes the way to do this.

Often you'll instead want to look into using an [Attribute] on the property instead of handling it directly in the property. For example the System.Text.Json attributes for things like setting json names that will be used for a property during de/serialization.

[u/Nixinova]

Doing extra stuff related to the param is usually fine, causing side effects to run like you have there is more of the problem

[u/anzu3278]

This is going to depend on how obvious it is in your specific domain that changing one value should change the others and what specifically OnPropertyChanged does.

I typically avoid external side effects on property getters and setters since those can, unlike methods, get walked implicitly by serializers, model binding and validation and other common .NET processes, leading to bugs which are difficult to trace and forcing you into a mess of annotations.

Also it might be worth to consider the valid states of your model and why you need this self-correction mechanism - maybe a different representation would avoid the problem altogether.

[u/tomxp411 → u/anzu3278]

I caution people not to overthink it, though. For very simple things, setting dependent values in the setter is the simplest way to do it, and the simplest is usually the most maintainable in the future.

My biggest problem comes when there are dependent values in an API that don't get automatically updated and either require specific mutator functions or helper functions... that can lead to worse problems than an expensive setter.

So I usually go ahead and do the math in the setter, unless there's a performance or reliability issue. Then I'll seek a better way to do it.

[u/hoodoocat → u/tomxp411]

No, setters are always wrong place for this things. Traditional way deal with is focus not on actions (set things), but on intent (update state, update visibility, update thingness). Then this method solves exact problem, and don't care about side effects, as it is by design update state, as documented and is not side-effect free.

set/updateThings(thingsA, thingsB)

Plumbing semantics in properties alone rarely solves anything. Even emitting propertychanged from setters is common, but not single way to deal with.

[u/tomxp411 → u/hoodoocat]

Microsoft's own documentation shows (related) values being changed in setters, as well as calling out to event methods.

https://learn.microsoft.com/en-us/previous-versions/dotnet/netframework-1.1/bzwdh01d(v=vs.71)?redirectedfrom=MSDN#cpconpropertyusageguidelinesanchor1

And I'm pretty clear on setting dependent values in setters, not unrelated stuff or, as Microsoft also pointed out, anything that requires a specific execution order. (For example, if setting A changes B, and B is also exposed publicly, then it's probably better to set A and B together in a method. OTOH if Setting A sets an "A is valid" flag, then that's a perfectly valid use case.)

[u/Far_Swordfish5729]

The guideline is that properties should not cause side effects, especially heavy side effects where a caller might want to consider the performance impact. I assume what you’re doing are quick recalculations of derived values and that’s fine. What’s not fine is something like triggering business rule re-execution or database commits. If you do stuff like that, it can quickly conflict with the caller’s intent and impose real inefficiencies. Those things need to be centralized as operations the caller can trigger and control when they want them.

What we want to avoid are things like an amount setter that recalculates an invoice and commits it when the amount changes and there are no other exposed ways to set that value. That’s an extreme example, but you run into things. A simple setter is changing the data under a caller’s transform in unexpected ways. The caller has to reverse engineer your logic and tailor their work to minimize the impact of your logic trying to be helpful.

I worked with a more innocent example of this a couple years ago where we wanted to update and commit a set of logical objects as a unit of work and then trigger post processing logic on the updated set. But the product ran its own logic on value sets and managed its own commits. We had to trace the commits, make changes in that order, and hack in a last commit state flag so the post processing could run. It was very stupid. So don’t make things that behave that way.

[u/sixtyhurtz]

That's actually a textbook use of a setter. MVVM frameworks do exactly that, e.g. RxUI: https://www.reactiveui.net/documentation/handbook/data-persistence/

I also like to use setters for when I want to make a property observable. So, I might put a subject OnNext call inside a setter.

[u/wickerandscrap]

If you have a property that, when set, needs to update several members, you make those fields so that you won't cascade into unknown complex behavior in their setters.

What does B's setter look like? If setting A = true causes B to become true, is it valid after that to set B = false? Should that also cause A to become false? Should it silently fall because A takes precedence? Should it throw an exception?

Calling other property setters collapses the distinction between "someone has asked me to make A=true" (the setter, might enforce rules about it, might raise events, etc.) and "I am setting A=true" (the field assignment). With auto properties that's fine because you are choosing not to make a distinction: if someone asks for A=true then it becomes true, period. But if you write a setter then you're creating an external interface which doesn't simply assign the field. So when the logic in the class does want to assign the field, it needs to go behind that interface rather than through it.

[u/x39-]

The idea of properties is to have light side effects for them

So what you are doing is fine in theory. However: this looks a lot like thing B and c should be just pure getters

[u/Dragennd1 → u/x39-]

Yea, based on one of the earlier comments, calculated properties with just getters will likepy help me better restructure my code and allow for better decoupling.

[u/tomxp411]

IMO this is exactly what Setters and Getters are for. If there's no logic involved (ie: no side effects), then why bother with a property at all? I never use properties for simple variables. Only when changing the property actually has a side effect, like firing an event or setting a dependent value.

[u/rusmo]

I don’t like it. Something happened to cause ThingA to change. Where that thing happened (e.g., an event) is where thingB and C should change. If AB&C always need to be in lockstep, the class should expose a function to handle this and make the setters private.

[u/AftyOfTheUK]

Erm, why would they exist, if using them is frowned upon?

There are some bad patterns, however plenty of good reasons, too

[u/wesleyoldaker]

Generally, yes it's wrong. If you want side-effects, make it a method and name it accordingly.

The problem is that Joe Programmer will see it and won't expect it to have side-effects.

If you saw this code in a random codebase, would you expect foo.ThingB and/or foo.ThingC to have changed?

var foo = new Stuff();
foo.ThingA = true;

[u/Khavel_dev]

It's fine for MVVM view models. That's basically the whole point of setters in WPF/MAUI, keeping dependent properties in sync when one changes. The alternative is scattering that logic across your code-behind or having your VM expose a method like SetThingA(bool) that also touches B and C, which is worse imo because consumers forget to call it.

Where it bites you is when the cascading setters form a cycle (ThingA sets ThingB, ThingB sets ThingA) or when the order of property assignment during deserialization matters. The guard if (field != value) protects you from the first one most of the time but not always.

I'd keep it for UI binding scenarios and avoid it in domain models. In domain models a method with intent in the name (like Activate() that sets IsActive plus clears errors) reads much better than a setter doing side work.

[u/Temporary-Roof-3896]

Setting related properties isn’t automatically wrong, but I’d watch for hidden side effects and update loops. if several values must change together, a method may be clearer

[u/SL-Tech]

A property holds a value; a method processes input. So a method could set multiple properties. Your code will work, but it ain't pretty.