Is this a proper use of properties or is this over complicated?

I’m starting to incorporate more properties into my classes and I would like to know if this is the proper use for them.

        private bool IsDisplayingEquipment
        {
            get => _isDisplayingEquipment;
            set
            {
                value = _instance.CurrentPlayerState != GameInstance.PlayerState.Interacting ? value : true; // If the player is in shop equipment must be active
                _isDisplayingEquipment = value; //
                _equipmentDisplay.IsDisplayed = value;
                if (value == true)
                {
                    _interactor.StartCheck(); //Start checking for mouse inputs
                }
                else
                {
                    _interactor.EndCheck(); //End check for mouse inputs
                }

            }
        }

The goal here is to have less code overall in order to achieve what I want without compromising readability. Before this I would have a function called SetEquipmentDisplay instead.

Worth remembering properties are just syntax sugar. At the end of the day, they’re still methods. When to use one or the other?

I think Microsoft cover this fairly comprehensively: Choosing Between Properties and Methods | Microsoft Learn

In general, methods represent actions and properties represent data. Properties are meant to be used like fields, meaning that properties should not be computationally complex or produce side effects. When it does not violate the following guidelines, consider using a property, rather than a method, because less experienced developers find properties easier to use.

The setter in this property very much feels like an action as opposed to merely mutating a simple value, with side effects outside what the data might imply. I would perhaps have a get-only property, and put the setter behind a clearly named method.

But it comes down to what feels right in the specific context. 99.99% of my properties are simply get-only properties to keep data read only.

public int SomeInt => _someInt;

And modifying objects is generally done through clearly named methods.

3 Likes

I think the reason has been covered well in the other reply. I will point out that I would never approve of such code if it was part of a pull request.

It not only does way too much for a setter it actually ignores the value passed in under some circumstances. That is unexpected behavior.

1 Like

I concur with this.

@billygamesinc
Don’t be clever with properties. Use them like logical extensions to readonly or read/write fields. Typically there are only two scenarios where properties are useful: 1) input validation, or 2) state validation (with potential side-effects).

The setter in the 2nd scenario can sometimes appear complicated. In that case, promote the setter’s body to a private method so that you can keep the property as simple as possible.

float _gridSpacing;
public float GridSpacing {
  get => _gridSpacing;
  set => setGridSpacing(value);
}

private void setGridSpacing(float value) {
  // validate/sanitize input i.e. value = Mathf.Max(0f, value);
  // invalidate/process some state(s) i.e. _gridObj.Invalidate();
  _gridSpacing = value;
  // propagate as needed i.e. _gridObj.SetSpacing(_gridSpacing);
  // yadda yadda
}

Similarly if all you want is to feature a read-only access, you can use auto-properties.
Instead of doing

public float _gridSpacing;

you do

public float GridSpacing { get; } // no backing field is needed
// + you can also initialize this by appending = 2.2; (i.e.)

If you want to internally be able to write, but still have it readonly then

public float GridSpacing { get; private set; }

That said, a property is really just a facade for a field (edit: a syntactic sugar, as stated before) with some advanced behavior attached to it, think of it like a reaction to user code trying to change a field, or a convenience of sorts. This includes getters as well.

float _angle; // private; in radians
public float Angle => _angle;
public float AngleDegrees => _angle * Mathf.Rad2Deg;

This one will escalate the situation upon detecting a change

float _gridSpacing;
public float GridSpacing {
  get => _gridSpacing;
  set {
    if(!isSame(_gridSpacing, value, threshold: 1E-4f)) {
      _gridSpacing = value;
      GridChanged?.Invoke(this, _gridSpacing);
    }
  }
}

If your get or set blocks/expressions violate the nominal behavior of actually getting or setting a backing field, then you shouldn’t really use properties, although there could be some exceptions to this if this fits into the above scenarios. For example

int _ni;
public int NaturalInteger {
  get => _ni;
  set => _ni = max(1, value);
}

Here doing myObj.NaturalInteger = -5 would implicitly assign 1 and that’s ok by design. Maybe you don’t like this design, and would like to throw an error instead, that’s also fine.

int _ni;
public int NaturalInteger {
  get => _ni;
  set {
    if(value <= 0) throw new ArgumentException();
    _ni = value;
  }
}

All of this, however, doesn’t mean the class fields are immediately rendered obsolete by properties. If you don’t mind a field being read-write and need not react to it being changed, then simply use a public field.

For more information refer to spiney’s post above.

2 Likes