• Sauron (unregistered)

    It's an interesting choice they named a variable "myParent". Since it's code about removing children, this gives off a quite unhealthy vibe.

    Also, frist.

  • (nodebb)

    And this code, ostensibly, removes a node in the middle and reparents its children

    No. I don't think it does. There's no code that reparents anything here.

    I think it's code to remover this (and all its children) from the tree. If this.Parent is not of class SomeClass that is what it does. However, if this.Parent is an instance of SomeClass then this.Parent and all its children gets removed from the tree. If SomeClass is a class that only allows exactly one child then the code makes sense. It can't have zero children, so it's got to go. It's still a WTF because of the broken encapsulation and the possibility that its parent is also of type SomeClass but it's nowhere near as bad as the article portrays.

    I also don't get the comments about memory management. This is C# isn't it? I thought C# garbage collected by walking the reference tree from the "roots" so, like Java, circular reference chains are not an issue. If there is no way to reach such a chain from the root, the objects in it will be collected.

  • (nodebb)

    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 don't think it matters. Once the child goes out of scope, both it and its parent will be unreachable (since the child is not added back to its "grandparent"), so they will be garbage collected. Unless there are other references to them somewhere, of course...

  • MaxiTB (unregistered)

    First up, for those asking, yes this code is C# code. My personal guess is either it's XmlElement related or WinForm related, because those are the two most common instances these days where you can see the old component model.

    var myParent = this.Parent; is a weird one, this should generally be avoided and this line will most likely result in a style violation warning.

    if (myParent is SomeClass asSomeClass) is a strong indicator that parent is a XmlNode and SomeClass is an XmlElement; but it could also be a Control which gets pattern matched, nothing wrong with this, that's a pretty common pattern actually, it's short for:

    var asSomeClass = myParent as SomeClass;
if(asSomeClass is not null)

    Now here comes the tricky part:

    myParent = myParent.Parent;
myParent.Children.Remove(asSomeClass);

    This is essentially Parent.Parent.Parent.Children.Remove(Parent.Parent) which confused the hell out of me. It removes something that should be in another branch of the component tree ... ehm.

    At this point, there is no point of continuing because this is either some very odd design convention resulting in completely unsafe code or this code was always buggy anyway.

    But maybe I miss something here and then we have another reason for refactoring :-)

  • Sole Purpose Of Visit (unregistered) in reply to Jeremy Pereira

    I think Remy may be getting confused between Mark & Sweep and Generational garbage collection.

    As far as I know, you are correct, and in Generational garbage collection, all circular references are just abandoned unless there is an external reference. (They're not so much abandoned as "left on the G1 or G2 or G3 heap," which is much the same thing.

    Not so sure about mark and sweep, though. And I could be wrong, anyway.

  • (nodebb) in reply to Sole Purpose Of Visit

    Both "Mark & Sweep" and "Stop & Copy" garbage collection handle circular references. Generational collection is an extension of "Stop & Copy" garbage collection. (I don't know of a "Mark & Sweep" generational collector, but I assume one could be written.) Reference Counting can't handle circular references.

  • Sole Purpose Of Visit (unregistered) in reply to WatersOfOblivion

    Well, a counting algorithm on steroids certainly can. Basically (if I have this right) you start two reference trackers at the same root, and increment one by a single reference, and the other by a two-deep reference. (I failed on this interview question, so I'm wide open here.). If at any stage the two trackers reference the same object, you have a circular reference.

    Whatever the details of this algorithm, I'm pretty sure the correct version would catch circularity. Obviously there are certain constraints, mostly because you'd use recursion (hopefully left tail), and I'm not saying it's an efficient way of catching circular references. But it basically works.

  • Sole Purpose Of Visit (unregistered) in reply to WatersOfOblivion

    As a side note, a mark & sweep generational garbage collector would be a waste of resources and time. (I am not a garbage collector, nor do I play one on TV.)

    The whole point of a generational garbage collector is based upon the observation that 90% of objects on a random heap have very short lifetimes. Thus, counter-intuitively, it is actually cheaper to go to "generation 2" by moving references to the next version of the heap and simply throwing away the original heap.

    Helps if you have bit-blatting and the ability to handle large objects, of course, and much more. But the principle is simple and robust. It's also a counter-argument to idiots who claim that a C# structure is somehow "cheaper" than a C# class.

  • (nodebb) in reply to Sole Purpose Of Visit

    Reference counting means that when you create a reference to an object, you increment an internal counter and when you remove a reference to an object you decrement the counter and delete the object if the counter gets to zero. You never focus on more than the immediate reference and you never update more than one reference counter at a time.

    You could, for example, have an object which "owns" a linked list of say 1,000 objects. If that list is not circular and the owning object has one reference to it, every object has a reference count of 1. When all the references to the owner object go away, its ref count goes to zero and it s deleted. As part of the deletion process it removes its reference to the first item in the list which causes that object's reference count to go to zero and so the thing cascades down the list until everything is gone.

    If you have the same scenario but you add the first object in the list to the end of the list. then it will have a reference count of 2, one from the owner and one from the tail of the list. When the owner loses its last reference, it will decrease the reference count on the first object in the list by 1, so the first object in the list will still have a reference cont of 1 even though all the references by which something external could access it have gone. You could, at this point use your counting algorithm to find out if you are in a reference cycle, but you'd have to do that every time you decrement a reference count to 1, which is not efficient. Also, an object can be in more than one reference cycles. A node designed for a binary tree can be in two reference cycles.

    Actual reference counting garbage collectors (like the one in Swift) use a concept called ownership to get around this issue. You would create unowned references i.e. ones that aren't counted for situations where reference cycles are required. Who'd have known there is a character limit WTF

  • WTFGuy (unregistered)

    (Hoping I can get the combo of quoting & code formatted correctly ...)

    @**MaxiTB ** ref:

    Now here comes the tricky part:

    myParent = myParent.Parent;
myParent.Children.Remove(asSomeClass);

    This is essentially Parent.Parent.Parent.Children.Remove(Parent.Parent) which confused the hell out of me. It removes something that should be in another branch of the component tree ... ehm.

    At this point, there is no point of continuing because this is either some very odd design convention resulting in completely unsafe code or this code was always buggy anyway.

    But maybe I miss something here and then we have another reason for refactoring :-)

    I think you've got one too many Parents in there. I read the effect as Parent.Parent.Children.Remove(Parent.Parent) or more colloquially myGrandParent.Children.Remove(myGrandParent)

  • Kordalia (unregistered) in reply to WTFGuy
    Comment held for moderation.
  • NoLand (unregistered) in reply to WTFGuy

    I think you've got one too many Parents in there. I read the effect as Parent.Parent.Children.Remove(Parent.Parent) or more colloquially myGrandParent.Children.Remove(myGrandParent)

    asSomeClass is still pointing to the original parent, it's myGrandParent.Children.Remove(myParent). I guess, the difficulties in deciphering this is the real WTF.

Leave a comment on “Bad Parenting”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article