X

Aggregator

CodeSOD: Property Flippers

The Daily WTF
3 weeks ago

Kleyguerth was having a hard time tracking down a bug. A _hasPicked flag was "magically" toggling itself to on. It was a bug introduced in a recent commit, but the commit in question was thousands of lines, and had the helpful comment "Fixed some stuff during the tests".

In several places, the TypeScript code checks a property like so:

if (!this.checkAndPick) { // do stuff }

Now, TypeScript, being a Microsoft language, allows properties to be just, well, properties, or it allows them to be functions with getters and setters.

You see where this is going. Once upon a time was a property that just checked another, private property, and returned its value, like so:

private get checkAndPick() { return this._hasPicked; }

Sane, reasonable choice. I have questions about why a private getter exists, but I'm not here to pick nits.

As we progress, someone changed it to this:

private get checkAndPick() { return this._hasPicked || (this._hasPicked = true); }

This forces the value to true, and returns true. This always returns true. I don't like it, because using a property accessor to mutate things is bad, but at least the property name is clear- checkAndPick tells us that an item is being picked. But what's the point of the check?

Still, this version worked as people expected it to. It took our fixer to take it to the next level:

private get checkAndPick() { return this._hasPicked || !(this._hasPicked = true); }

This flips _hasPicked to true if it's not already true- but if it does, returs false. The badness of this code is rooted in the badness of the previous version, because a property should never be used this way. And while this made our fixer's tests turn green, it broke everything for everyone else.

Also: do not, do not use property accessors to mutate state. Only setters should mutate state, and even then, they should only set a field based on their input. Complicated logic does not belong in properties. Or, as this case shows, even simple logic doesn't, if that simple logic is also stupid.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
Remy Porter