Updated to 1.4.0

Author: Oren (electricessence)

Node.Factory.cs

@@ -1,5 +1,6 @@
 using Open.Disposable;
 using System;
+using System.Diagnostics.Contracts;
 
 namespace Open.Hierarchy
 {
@@ -14,7 +15,7 @@ public sealed class Factory : DisposableBase
 			public Factory()
 			{
 				_pool = new ConcurrentQueueObjectPool<Node<T>>(
-					() => new Node<T>(), PrepareForPool, null, ushort.MaxValue);
+					() => new Node<T>(this), PrepareForPool, null, ushort.MaxValue);
 			}
 
 			protected override void OnDispose(bool calledExplicitly)
@@ -33,21 +34,28 @@ protected override void OnDispose(bool calledExplicitly)
 			/// <returns>A blank node.</returns>
 			public Node<T> GetBlankNode()
 			{
+				var p = _pool;
 				AssertIsAlive();
 
-				return _pool?.Take();
+				var n = p.Take();
+				n._recycled = false;
+				return n;
 			}
 
+
 			/// <summary>
 			/// Recycles the node into the object pool, returning the value contained.
 			/// </summary>
-			/// <param name="n">The node to be recycled.</param>
+			/// <param name="node">The node to be recycled.</param>
 			/// <returns>The value contained in the node.</returns>
-			public T Recycle(Node<T> n)
+			public T Recycle(Node<T> node)
 			{
-				AssertIsAlive();
+				if (node == null) throw new ArgumentNullException(nameof(node));
+				if (node._factory != this)
+					throw new ArgumentException("The node being provided for recycling does not belong to this factory.", nameof(node));
+				Contract.EndContractBlock();
 
-				return RecycleInternal(n);
+				return RecycleInternal(node);
 			}
 
 			internal T RecycleInternal(Node<T> n)
@@ -59,8 +67,12 @@ internal T RecycleInternal(Node<T> n)
 				return value;
 			}
 
-			void PrepareForPool(Node<T> n)
-				=> n.Recycle(this);
+			static void PrepareForPool(Node<T> n)
+			{
+				n.Recycle();
+				n._recycled = true;
+			}
+
 			#endregion
 
 			/// <summary>
@@ -78,9 +90,16 @@ public Node<T> Clone(
 				Node<T> newParentForClone = null,
 				Action<Node<T>, Node<T>> onNodeCloned = null)
 			{
+				if (target == null) throw new ArgumentNullException(nameof(target));
+				if (target._factory != this)
+					throw new ArgumentException("The node being provided for cloning does not belong to this factory.", nameof(target));
+				if (newParentForClone != null && newParentForClone._factory != this)
+					throw new ArgumentException("The node being provided for cloning does not belong to this factory.", nameof(newParentForClone));
+				Contract.EndContractBlock();
+
 				AssertIsAlive();
 
-				var clone = _pool.Take();
+				var clone = GetBlankNode();
 				clone.Value = target.Value;
 				newParentForClone?.Add(clone);
 
@@ -92,7 +111,6 @@ public Node<T> Clone(
 				return clone;
 			}
 
-
 			/// <summary>
 			/// Clones a node by recreating the tree and copying the values.
 			/// </summary>
@@ -102,9 +120,7 @@ public Node<T> Clone(
 			public Node<T> Clone(
 				Node<T> target,
 				Action<Node<T>, Node<T>> onNodeCloned)
-			{
-				return Clone(target, null, onNodeCloned);
-			}
+				=> Clone(target, null, onNodeCloned);
 
 			/// <summary>
 			/// Create's a clone of the entire tree but only returns the clone of this node.
@@ -132,14 +148,13 @@ public Node<T> Map<TRoot>(TRoot root)
 			{
 				AssertIsAlive();
 
-				var current = _pool.Take();
+				var current = GetBlankNode();
 				current.Value = root;
 
-				if (!(root is IParent<T> parent)) return current;
-				foreach (var child in parent.Children)
-				{
-					current.Add(Map<T>(child));
-				}
+				// Mapping is deferred and occurs on demand.
+				// If values or children change in the node, mapping is disregarded.
+				current._needsMapping = true;
+
 
 				return current;
 			}
@@ -150,10 +165,7 @@ public Node<T> Map<TRoot>(TRoot root)
 			/// </summary>
 			/// <param name="root">The root instance.</param>
 			/// <returns>The full map of the root.</returns>
-			public Node<T> Map(T root)
-			{
-				return Map<T>(root);
-			}
+			public Node<T> Map(T root) => Map<T>(root);
 
 			/// <summary>
 			/// Generates a full hierarchy if the root of the container is an IParent and uses the root as the value of the hierarchy.
@@ -163,10 +175,7 @@ public Node<T> Map(T root)
 			/// <param name="container">The container of the root instance.</param>
 			/// <returns>The full map of the root.</returns>
 			public Node<T> Map<TRoot>(IHaveRoot<TRoot> container)
-				where TRoot : T
-			{
-				return Map(container.Root);
-			}
+				where TRoot : T => Map(container.Root);
 
 		}
 

Node.cs

@@ -12,22 +12,65 @@ public sealed partial class Node<T> : INode<Node<T>>
 
 		#region IParent<Node<T>> Implementation
 		private readonly List<Node<T>> _children;
+		private readonly IReadOnlyList<Node<T>> _childrenReadOnly;
+
 		/// <inheritdoc />
-		public IReadOnlyList<Node<T>> Children { get; }
+		public IReadOnlyList<Node<T>> Children => EnsureChildrenMapped();
 		/// <inheritdoc />
-		IReadOnlyList<object> IParent.Children => Children;
+		IReadOnlyList<object> IParent.Children => EnsureChildrenMapped();
 		#endregion
 
+		private bool _recycled;
+		void AssertNotRecycled()
+		{
+			if (_recycled)
+				throw new InvalidOperationException("Attempting to modify a node that has been recyled.");
+		}
+
 		// ReSharper disable once UnusedAutoPropertyAccessor.Global
 		/// <summary>
 		/// The value for the node to hold on to.
 		/// </summary>
-		public T Value { get; set; }
+		private T _value;
+		public T Value
+		{
+			get => _value;
+			set
+			{
+				AssertNotRecycled();
+				_needsMapping = false;
+				_value = value;
+			}
+		}
+
+		private readonly Factory _factory;
+		private bool _needsMapping;
+
+		IReadOnlyList<Node<T>> EnsureChildrenMapped()
+		{
+			if (!_needsMapping)
+				return _childrenReadOnly;
+
+			// Need to avoid double mapping and this method is primarily called when 'reading' from the node and contention will only occur if mapping is needed.
+			lock (_children)
+			{
+				if (!_needsMapping) return _childrenReadOnly;
+				if (_value is IParent<T> p)
+				{
+					foreach (var child in p.Children)
+						_children.Add(_factory.Map(child));
+				}
+				_needsMapping = false;
+			}
+
+			return _childrenReadOnly;
+		}
 
-		private Node()
+		internal Node(Factory factory)
 		{
+			_factory = factory ?? throw new ArgumentNullException(nameof(factory));
 			_children = new List<Node<T>>();
-			Children = _children.AsReadOnly();
+			_childrenReadOnly = _children.AsReadOnly();
 		}
 
 		// WARNING: Care must be taken not to have duplicate nodes anywhere in the tree but having duplicate values are allowed.
@@ -52,6 +95,9 @@ public bool Remove(Node<T> node)
 			Contract.EndContractBlock();
 
 			if (!_children.Remove(node)) return false;
+
+			AssertNotRecycled();
+			_needsMapping = false;
 			node.Parent = null; // Need to be very careful about retaining parent references as it may cause a 'leak' per-se.
 			return true;
 		}
@@ -68,13 +114,20 @@ public void Add(Node<T> node)
 					throw new InvalidOperationException("Adding a child node more than once.");
 				throw new InvalidOperationException("Adding a node that belongs to another parent.");
 			}
+
+			AssertNotRecycled();
+			EnsureChildrenMapped(); // Adding to potentially existing nodes.
 			node.Parent = this;
 			_children.Add(node);
 		}
 
 		/// <inheritdoc />
 		public void Clear()
 		{
+			if (_children.Count == 0) return;
+
+			AssertNotRecycled();
+			_needsMapping = false;
 			foreach (var c in _children)
 				c.Parent = null;
 			_children.Clear();
@@ -86,15 +139,18 @@ public void Clear()
 
 		/// <inheritdoc />
 		public IEnumerator<Node<T>> GetEnumerator()
-			=> _children.GetEnumerator();
+			=> Children.GetEnumerator();
 
 		/// <inheritdoc />
 		IEnumerator IEnumerable.GetEnumerator()
 			=> GetEnumerator();
 
 		/// <inheritdoc />
 		public void CopyTo(Node<T>[] array, int arrayIndex)
-			=> _children.CopyTo(array, arrayIndex);
+		{
+			EnsureChildrenMapped();
+			_children.CopyTo(array, arrayIndex);
+		}
 		#endregion
 
 		/// <summary>
@@ -113,6 +169,9 @@ public void Replace(Node<T> node, Node<T> replacement)
 			var i = _children.IndexOf(node);
 			if (i == -1)
 				throw new InvalidOperationException("Node being replaced does not belong to this parent.");
+
+			AssertNotRecycled();
+			_needsMapping = false;
 			_children[i] = replacement;
 			node.Parent = null;
 			replacement.Parent = this;
@@ -123,6 +182,7 @@ public void Replace(Node<T> node, Node<T> replacement)
 		/// </summary>
 		public void Detatch()
 		{
+			AssertNotRecycled();
 			Parent?.Remove(this);
 			Parent = null;
 		}
@@ -149,6 +209,7 @@ public Node<T> Root
 		/// </summary>
 		public void Teardown()
 		{
+			_needsMapping = false;
 			Value = default;
 			Detatch(); // If no parent then this does nothing...
 			TeardownChildren();
@@ -159,6 +220,10 @@ public void Teardown()
 		/// </summary>
 		public void TeardownChildren()
 		{
+			if (_children.Count == 0) return;
+
+			AssertNotRecycled();
+			_needsMapping = false;
 			foreach (var c in _children)
 			{
 				c.Parent = null; // Don't initiate a 'Detach' (which does a lookup) since we are clearing here;
@@ -170,35 +235,30 @@ public void TeardownChildren()
 		/// <summary>
 		/// Recycles this node.
 		/// </summary>
-		/// <param name="factory">The factory to use as a recycler.</param>
 		// ReSharper disable once UnusedMethodReturnValue.Global
-		public T Recycle(Factory factory)
+		public T Recycle()
 		{
-			if (factory == null)
-				throw new ArgumentNullException(nameof(factory));
-			Contract.EndContractBlock();
-
+			AssertNotRecycled(); // Avoid double entry in the pool.
+			_needsMapping = false;
 			var value = Value;
 			Value = default;
 			Detatch(); // If no parent then this does nothing...
-			RecycleChildren(factory);
+			RecycleChildren();
 			return value;
 		}
 
 		/// <summary>
 		/// Recycles all the children of this node.
 		/// </summary>
-		/// <param name="factory">The factory to use as a recycler.</param>
-		public void RecycleChildren(Factory factory)
+		public void RecycleChildren()
 		{
-			if (factory == null)
-				throw new ArgumentNullException(nameof(factory));
-			Contract.EndContractBlock();
+			if (_children.Count == 0) return;
 
+			_needsMapping = false;
 			foreach (var c in _children)
 			{
 				c.Parent = null; // Don't initiate a 'Detach' (which does a lookup) since we are clearing here;
-				factory.RecycleInternal(c);
+				_factory.RecycleInternal(c);
 			}
 			_children.Clear();
 		}

Open.Hierarchy.csproj

@@ -14,8 +14,10 @@ Part of the "Open" set of libraries.</Description>
     <RepositoryUrl>https://github.com/electricessence/Open.Hierarchy/</RepositoryUrl>
     <RepositoryType>git</RepositoryType>
     <PackageTags>dotnet, dotnetcore, cs, tree, node, hierarchy, parent, child, root</PackageTags>
-    <Version>1.3.0</Version>
-    <PackageReleaseNotes>Cleanup and inspection with Resharper.</PackageReleaseNotes>
+    <Version>1.4.0</Version>
+    <PackageReleaseNotes>Deferred unidirectional to bidirectional tree mapping.
+Improved assertions and added recycled node validation.
+Simplified factory and recycling usage: nodes now have a referene to their source factory.</PackageReleaseNotes>
   </PropertyGroup>
 
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">

TraversalExtensions.cs

@@ -6,7 +6,7 @@ namespace Open.Hierarchy
 {
 	public static class TraversalExtensions
 	{
-		// NOTE: super recursive, possibly could be better, but this is a unidirectional heirarchy so it's not so easy to avoid recursion.
+		// NOTE: not recursive, but could produce a large stack while traversing, possibly could be better, but this is a unidirectional heirarchy so it's not so easy to avoid recursion.
 
 		/// <summary>
 		/// Returns all descendant nodes.