From cd5d9bd5b90d8a9aa563362b0ed78906debf45b3 Mon Sep 17 00:00:00 2001
From: Cameron Fischer <fischer.cameron@gmail.com>
Date: Wed, 9 Dec 2020 04:46:35 -0500
Subject: [PATCH] Code cleanup of Linked Lists (#5241)

* made private methods limited to module scope
* moved private methods to file bottom
* changed tests to run comperable array functions in parallel
* added comments
---
 core/modules/utils/linked-list.js             |  56 ++++-----
 .../test/tiddlers/tests/test-linked-list.js   | 118 +++++++++++-------
 2 files changed, 99 insertions(+), 75 deletions(-)

diff --git a/core/modules/utils/linked-list.js b/core/modules/utils/linked-list.js
index 50421ab56..b25f0b8fd 100644
--- a/core/modules/utils/linked-list.js
+++ b/core/modules/utils/linked-list.js
@@ -24,40 +24,19 @@ LinkedList.prototype.clear = function() {
 LinkedList.prototype.remove = function(value) {
 	if($tw.utils.isArray(value)) {
 		for(var t=0; t<value.length; t++) {
-			this._removeOne(value[t]);
+			_removeOne(this,value[t]);
 		}
 	} else {
-		this._removeOne(value);
+		_removeOne(this,value);
 	}
 };
 
-LinkedList.prototype._removeOne = function(value) {
-	var node = this.index[value];
-	if(node) {
-		node.prev.next = node.next;
-		node.next.prev = node.prev;
-		this.length -= 1;
-		// Point index to the next instance of the same value, maybe nothing.
-		this.index[value] = node.copy;
-	}
-	return node;
-};
-
-LinkedList.prototype._linkToEnd = function(node) {
-	// Sticks the given node onto the end of the list.
-	this.prev.next = node;
-	node.prev = this.prev;
-	this.prev = node;
-	node.next = this;
-	this.length += 1;
-};
-
 LinkedList.prototype.push = function(/* values */) {
 	for(var i = 0; i < arguments.length; i++) {
 		var value = arguments[i];
 		var node = {value: value};
 		var preexistingNode = this.index[value];
-		this._linkToEnd(node);
+		_linkToEnd(this,node);
 		if(preexistingNode) {
 			// We want to keep pointing to the first instance, but we want
 			// to have that instance (or chain of instances) point to the
@@ -75,11 +54,11 @@ LinkedList.prototype.push = function(/* values */) {
 LinkedList.prototype.pushTop = function(value) {
 	if($tw.utils.isArray(value)) {
 		for(var t=0; t<value.length; t++) {
-			this._removeOne(value[t]);
+			_removeOne(this,value[t]);
 		}
-		this.push.apply(this, value);
+		this.push.apply(this,value);
 	} else {
-		var node = this._removeOne(value);
+		var node = _removeOne(this,value);
 		if(!node) {
 			node = {value: value};
 			this.index[value] = node;
@@ -95,7 +74,7 @@ LinkedList.prototype.pushTop = function(value) {
 			this.index[value] = node.copy;
 			node.copy = undefined;
 		}
-		this._linkToEnd(node);
+		_linkToEnd(this,node);
 	}
 };
 
@@ -113,6 +92,27 @@ LinkedList.prototype.toArray = function() {
 	return output;
 };
 
+function _removeOne(list,value) {
+	var node = list.index[value];
+	if(node) {
+		node.prev.next = node.next;
+		node.next.prev = node.prev;
+		list.length -= 1;
+		// Point index to the next instance of the same value, maybe nothing.
+		list.index[value] = node.copy;
+	}
+	return node;
+};
+
+function _linkToEnd(list,node) {
+	// Sticks the given node onto the end of the list.
+	list.prev.next = node;
+	node.prev = list.prev;
+	list.prev = node;
+	node.next = list;
+	list.length += 1;
+};
+
 exports.LinkedList = LinkedList;
 
 })();
diff --git a/editions/test/tiddlers/tests/test-linked-list.js b/editions/test/tiddlers/tests/test-linked-list.js
index c66a1fc3b..0cef342d0 100644
--- a/editions/test/tiddlers/tests/test-linked-list.js
+++ b/editions/test/tiddlers/tests/test-linked-list.js
@@ -8,6 +8,10 @@ Tests the utils.LinkedList class.
 LinkedList was built to behave exactly as $tw.utils.pushTop and
 Array.prototype.push would behave with an array.
 
+Many of these tests function by performing operations on a LinkedList while
+performing the equivalent actions on an array with the old utility methods.
+Then we confirm that the two come out functionally identical.
+
 \*/
 (function(){
 
@@ -17,66 +21,88 @@ Array.prototype.push would behave with an array.
 
 describe("LinkedList class tests", function() {
 
+	// pushTops a value or array of values into both the array and linked list.
+	function pushTop(array, linkedList, valueOrValues) {
+		$tw.utils.pushTop(array, valueOrValues);
+		linkedList.pushTop(valueOrValues);
+	};
+
+	// pushes values into both the array and the linked list.
+	function push(array, linkedList/*, other values */) {
+		var values = Array.prototype.slice(arguments, 2);
+		array.push.apply(array, values);
+		linkedList.push.apply(linkedList, values);
+	};
+
+	// operates a remove action on an array and a linked list in parallel.
+	function remove(array, linkedList, valueOrValues) {
+		$tw.utils.removeArrayEntries(array, valueOrValues);
+		linkedList.remove(valueOrValues);
+	};
+
+	// compares an array and a linked list to make sure they match up
+	function compare(array, linkedList) {
+		expect(linkedList.toArray()).toEqual(array);
+		expect(linkedList.length).toBe(array.length);
+	};
+
 	it("can pushTop", function() {
+		var array = [];
 		var list = new $tw.utils.LinkedList();
-		list.push('A', 'B', 'C');
+		push(array, list, 'A', 'B', 'C');
 		// singles
-		list.pushTop('X');
-		list.pushTop('B');
-		expect(list.toArray()).toEqual(['A', 'C', 'X', 'B']);
-		expect(list.length).toBe(4);
+		pushTop(array, list, 'X');
+		pushTop(array, list, 'B');
+		compare(array, list); // A C X B
 		//arrays
-		list.pushTop(['X', 'A', 'G', 'A']);
+		pushTop(array, list, ['X', 'A', 'G', 'A']);
 		// If the pushedTopped list has duplicates, they go in unempeded.
-		expect(list.toArray()).toEqual(['C', 'B', 'X', 'A', 'G', 'A']);
-		expect(list.length).toBe(6);
+		compare(array, list); // C B X A G A
 	});
 
 	it("can pushTop with tricky duplicates", function() {
+		var array = [];
 		var list = new $tw.utils.LinkedList();
-		list.push('A', 'B', 'A', 'C', 'A', 'end');
+		push(array, list, 'A', 'B', 'A', 'C', 'A', 'end');
 		// If the original list contains duplicates, only one instance is cut
-		list.pushTop('A');
-		expect(list.toArray()).toEqual(['B', 'A', 'C', 'A', 'end', 'A']);
-		expect(list.length).toBe(6);
+		pushTop(array, list, 'A');
+		compare(array, list); // B A C A end A
 
 		// And the Llist properly knows the next 'A' to cut if pushed again
-		list.pushTop(['X', 'A']);
-		expect(list.toArray()).toEqual(['B', 'C', 'A', 'end', 'A', 'X', 'A']);
-		expect(list.length).toBe(7);
+		pushTop(array, list, ['X', 'A']);
+		compare(array, list); // B C A end A X A
 
 		// One last time, to make sure we maintain the linked chain of copies
-		list.pushTop('A');
-		expect(list.toArray()).toEqual(['B', 'C', 'end', 'A', 'X', 'A', 'A']);
-		expect(list.length).toBe(7);
+		pushTop(array, list, 'A');
+		compare(array, list); // B C end A X A A
 	});
 
 	it("can handle particularly nasty pushTop pitfall", function() {
+		var array = [];
 		var list = new $tw.utils.LinkedList();
-		list.push('A', 'B', 'A', 'C');
-		list.pushTop('A'); // BACA
-		list.pushTop('X'); // BACAX
-		list.remove('A');  // BCAX
-		list.pushTop('A'); // BCXA
-		list.remove('A');  // BCX
+		push(array, list, 'A', 'B', 'A', 'C');
+		pushTop(array, list, 'A'); // BACA
+		pushTop(array, list, 'X'); // BACAX
+		remove(array, list, 'A');  // BCAX
+		pushTop(array, list, 'A'); // BCXA
+		remove(array, list, 'A');  // BCX
+
 		// But! The way I initially coded the copy chains, a mystery A could
 		// hang around.
-		expect(list.toArray()).toEqual(['B', 'C', 'X']);
-		expect(list.length).toBe(3);
+		compare(array, list); // B C X
 	});
 
 	it("can push", function() {
+		var array = [];
 		var list = new $tw.utils.LinkedList();
-		list.push('A', 'B', 'C');
+		push(array, list, 'A', 'B', 'C');
 		// singles
-		list.push('B');
-		expect(list.toArray()).toEqual(['A', 'B', 'C', 'B']);
-		expect(list.length).toBe(4);
+		push(array, list, 'B');
+		compare(array, list); // A B C B
 
 		// multiple args
-		list.push('A', 'B', 'C');
-		expect(list.toArray()).toEqual(['A', 'B', 'C', 'B', 'A', 'B', 'C']);
-		expect(list.length).toBe(7);
+		push(array, list, 'A', 'B', 'C');
+		compare(array, list); // A B C B A B C
 	});
 
 	it("can clear", function() {
@@ -88,31 +114,29 @@ describe("LinkedList class tests", function() {
 	});
 
 	it("can remove", function() {
+		var array = [];
 		var list = new $tw.utils.LinkedList();
-		list.push('A', 'x', 'C', 'x', 'D', 'x', 'E', 'x');
+		push(array, list, 'A', 'x', 'C', 'x', 'D', 'x', 'E', 'x');
 		// single
-		list.remove('x');
-		expect(list.toArray()).toEqual(['A', 'C', 'x', 'D', 'x', 'E', 'x']);
-		expect(list.length).toBe(7);
+		remove(array, list, 'x');
+		compare(array, list); // A C x D x E x
 
 		// arrays
-		list.remove(['x', 'A', 'x']);
-		expect(list.toArray()).toEqual(['C', 'D', 'E', 'x']);
-		expect(list.length).toBe(4);
+		remove(array, list, ['x', 'A', 'x']);
+		compare(array, list); // C D E x
 	});
 
 	it('can ignore removal of nonexistent items', function() {
+		var array = [];
 		var list = new $tw.utils.LinkedList();
-		list.push('A', 'B', 'C', 'D');
+		push(array, list, 'A', 'B', 'C', 'D');
 		// single
-		list.remove('Z');
-		expect(list.toArray()).toEqual(['A', 'B', 'C', 'D']);
-		expect(list.length).toBe(4);
+		remove(array, list, 'Z');
+		compare(array, list); // A B C D
 
 		// array
-		list.remove(['Z', 'B', 'X']);
-		expect(list.toArray()).toEqual(['A', 'C', 'D']);
-		expect(list.length).toBe(3);
+		remove(array, list, ['Z', 'B', 'X']);
+		compare(array, list); // A C D
 	});
 
 	it('can iterate with each', function() {