Hi, On Wed, Feb 29, 2012 at 1:54 AM, Michael Dürig <[email protected]> wrote: > See https://gist.github.com/1934912 for what I meant about factoring > mutability out for transient modifications into a decorator on top of a > completely immutable (i.e. persistent) Tree.
I see where you're going, but with "MutableTree implements Tree" (i.e. MutableTree ISA Tree) we still fail to make a categorical statement that "all Trees are immutable". In other words all you gain is a marker type for making it easier to track whether a given Tree instance is supposed to be mutable or not. Using the Factory (or Builder) pattern as you originally proposed would IMHO be better from a conceptual perspective, but that'll make it harder for something like a transient space to read content that's still being modified. And whichever way we decide, it should be an interface instead of a concrete class. I also notice you replaced "extends Map" with a set of custom method signatures in Tree. As discussed I'm fine with this change, but there are now some subtle issues that should be addressed: * By dropping signatures like entrySet() and values(), you force all subtree access to happen by string lookups. That might have a negative performance impact on some access patterns (for example a garbage collector that just needs to traverse the full tree). * Returning an Iterator instead of an Iterable from getChildNames() makes it unusable in foreach loops. * The asLeaf() and accept() methods are now somewhat overloaded. I'd only put one of them in the interface, and implement the other as a utility method in a separate class. BR, Jukka Zitting
