CodeSOD: Bad Parenting

Conditionals are hard. So are data structures. And today's anonymous submission gives us both of those.

var myParent = this.Parent;bool parentRemoved = false;if (myParent is SomeClass asSomeClass){myParent = myParent.Parent;myParent.Children.Remove(asSomeClass);parentRemoved = true;}if (!parentRemoved){myParent.Children.Remove(this);}

There are a few pieces to this one, so let's start by focusing in on the conditional expressions first:

if (myParent is SomeClass asSomeClass){ parentRemoved = true;}if (!parentRemoved){}

This is an else. I mean, it isn't an else, but logically, you could just write this as an if-else expression. This is just a head scratcher, but perhaps not the largest WTF ever. Also note the syntactic sugar- asSomeClass is a variable declared and assigned in the if statement, scoped to the first block.

The data-structure element is much worse. You can sort of infer it from the code, but our submitter makes it explicit. This code is part of the implementation of a tree, and the tree has bidirectional links between nodes- children reference their parent, parents reference their children. And this code, ostensibly, removes a node in the middle and reparents its children- but it does it all wrong, because it doesn't actually update the parent references. At most, it stores them in a temporary variable.

The more you look at it, the less sense the choices make. There's no accessor or methods for interacting with the tree as a data structure- this code reaches directly into properties of its parent node, which is just bad encapsulation. That is SomeClass check is clearly meant to implement polymorphism without actually using inheritance, which is bad- but also, the logical flow is "if the parent is a certain kind of node, delete the parent and reparent the child (without doing it correctly), but if the parent isn't that kind of class, just delete the child" which is... a confusing choice.

And finally, this likely wreaks havoc on garbage collection. While the grandparent releases its reference to its child, the node we're operating on never releases its reference to the parent. Not only does the tree rapidly lose integrity, it also loses any sort of coherent memory management.

I'm suspicious that maybe a developer implementing their own tree data structure instead of using a built-in one might be going down the wrong path. Our submitter closes out the story thus:

I'm working on replacing all these edits with use of accessor methods that ensure tree integrity... and it is... involving a lot more files than I wanted.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.