Skip to content

Commit

Permalink
Add better guard against inserting item from other list into linked list
Browse files Browse the repository at this point in the history 
Adds `__parent` property to items to identify their inclusion in a list.
This allows us to always know when we are inserting an item from another
list. The previous heuristic (checking for presence of `next` and `prev`) is
not correct in all scenarios, leading to unusual bugs.

This refactors `insertBefore` method slightly for clarity, and
simplifies `remove` because we now know for sure when we are removing
an item in this list.

Adds some additional test coverage.
  • Loading branch information
@bantic
bantic committed Dec 2, 2015
1 parent 38736a0 commit f7a4ef2
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 82 deletions.
126 changes: 68 additions & 58 deletions src/js/utils/linked-list.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import assert from './assert';

const PARENT_PROP = '__parent';

export default class LinkedList {
constructor(options) {
this.head = null;
Expand All @@ -11,9 +15,13 @@ export default class LinkedList {
}
}
adoptItem(item) {
item[PARENT_PROP]= this;
this.length++;
if (this._adoptItem) { this._adoptItem(item); }
}
freeItem(item) {
item[PARENT_PROP] = null;
this.length--;
if (this._freeItem) { this._freeItem(item); }
}
get isEmpty() {
Expand All @@ -26,81 +34,75 @@ export default class LinkedList {
this.insertBefore(item, null);
}
insertAfter(item, prevItem) {
let nextItem = null;
if (prevItem) {
nextItem = prevItem.next;
} else {
nextItem = this.head;
}
let nextItem = prevItem ? prevItem.next : this.head;
this.insertBefore(item, nextItem);
}
insertBefore(item, nextItem) {
if (item.next || item.prev || this.head === item) {
throw new Error('Cannot insert an item into a list if it is already in a list');
}
this._ensureItemIsNotInList(item);
this.adoptItem(item);

let insertPos;
if (nextItem && nextItem.prev) {
// middle of the items
let prevItem = nextItem.prev;
item.next = nextItem;
nextItem.prev = item;
item.prev = prevItem;
prevItem.next = item;
insertPos = 'middle';
} else if (nextItem) {
// first item
if (this.head === nextItem) {
item.next = nextItem;
nextItem.prev = item;
} else {
this.tail = item;
}
this.head = item;
insertPos = 'start';
} else {
// last item
if (this.tail) {
item.prev = this.tail;
this.tail.next = item;
}
if (!this.head) {
insertPos = 'end';
}

switch (insertPos) {
case 'start':
if (this.head) {
item.next = this.head;
this.head.prev = item;
}
this.head = item;
}
this.tail = item;

break;
case 'middle':
let prevItem = nextItem.prev;
item.next = nextItem;
item.prev = prevItem;
nextItem.prev = item;
prevItem.next = item;

break;
case 'end':
let tail = this.tail;
item.prev = tail;

if (tail) {
tail.next = item;
} else {
this.head = item;
}
this.tail = item;

break;
}
this.length++;
}
remove(item) {
if (!item[PARENT_PROP]) {
return;
}
this._ensureItemIsInThisList(item);
this.freeItem(item);

let didRemove = false;
if (item.next && item.prev) {
// Middle of the list
item.next.prev = item.prev;
item.prev.next = item.next;
didRemove = true;
let [prev, next] = [item.prev, item.next];
item.prev = null;
item.next = null;

if (prev) {
prev.next = next;
} else {
if (item === this.head) {
// Head of the list
if (item.next) {
item.next.prev = null;
}
this.head = item.next;
didRemove = true;
}
if (item === this.tail) {
// Tail of the list
if (item.prev) {
item.prev.next = null;
}
this.tail = item.prev;
didRemove = true;
}
this.head = next;
}
if (didRemove) {
this.length--;

if (next) {
next.prev = prev;
} else {
this.tail = prev;
}
item.prev = null;
item.next = null;
}
forEach(callback) {
let item = this.head;
Expand Down Expand Up @@ -190,4 +192,12 @@ export default class LinkedList {
item = nextItem;
}
}
_ensureItemIsNotInList(item) {
assert('Cannot insert an item into a list if it is already in a list',
!item[PARENT_PROP]);
}
_ensureItemIsInThisList(item) {
assert('Cannot remove item that is in another list',
item[PARENT_PROP] === this);
}
}
110 changes: 86 additions & 24 deletions tests/unit/utils/linked-list-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ const {module, test} = QUnit;
import LinkedList from 'mobiledoc-kit/utils/linked-list';
import LinkedItem from 'mobiledoc-kit/utils/linked-item';

const INSERTION_METHODS = ['append', 'prepend', 'insertBefore', 'insertAfter'];

module('Unit: Utils: LinkedList');

test('initial state', (assert) => {
Expand All @@ -13,7 +15,7 @@ test('initial state', (assert) => {
assert.equal(list.isEmpty, true, 'isEmpty is true');
});

['append', 'prepend', 'insertBefore', 'insertAfter'].forEach(method => {
INSERTION_METHODS.forEach(method => {
test(`#${method} initial item`, (assert) => {
let list = new LinkedList();
let item = new LinkedItem();
Expand All @@ -26,7 +28,7 @@ test('initial state', (assert) => {
assert.equal(item.prev, null, 'item prev is null');
});

test(`#${method} call adoptItem`, (assert) => {
test(`#${method} calls adoptItem`, (assert) => {
let adoptedItem;
let list = new LinkedList({
adoptItem(item) {
Expand All @@ -37,6 +39,29 @@ test('initial state', (assert) => {
list[method](item);
assert.equal(adoptedItem, item, 'item is adopted');
});

test(`#${method} throws if item is in this list`, (assert) => {
let list = new LinkedList();
let item = new LinkedItem();
list[method](item);

assert.throws(
() => list[method](item),
/Cannot insert.*already in a list/
);
});

test(`#${method} throws if item is in another list`, (assert) => {
let [list, otherList] = [new LinkedList(), new LinkedList()];
let [item, otherItem] = [new LinkedItem(), new LinkedItem()];
list[method](item);
otherList[method](otherItem);

assert.throws(
() => list[method](otherItem),
/Cannot insert.*already in a list/
);
});
});

test(`#append second item`, (assert) => {
Expand All @@ -54,7 +79,7 @@ test(`#append second item`, (assert) => {
assert.equal(itemTwo.next, null, 'itemTwo next is null');
});

test(`#prepend first item`, (assert) => {
test(`#prepend additional item`, (assert) => {
let list = new LinkedList();
let itemOne = new LinkedItem();
let itemTwo = new LinkedItem();
Expand Down Expand Up @@ -88,6 +113,22 @@ test(`#insertBefore a middle item`, (assert) => {
assert.equal(itemThree.next, null, 'itemThree next is null');
});

test('#insertBefore null reference item appends the item', (assert) => {
let list = new LinkedList();
let item1 = new LinkedItem();
let item2 = new LinkedItem();
list.append(item1);
list.insertBefore(item2, null);

assert.equal(list.length, 2);
assert.equal(list.tail, item2, 'item2 is appended');
assert.equal(list.head, item1, 'item1 is at head');
assert.equal(item2.prev, item1, 'item2.prev');
assert.equal(item1.next, item2, 'item1.next');
assert.equal(item2.next, null);
assert.equal(item1.prev, null);
});

test(`#insertAfter a middle item`, (assert) => {
let list = new LinkedList();
let itemOne = new LinkedItem();
Expand All @@ -96,6 +137,8 @@ test(`#insertAfter a middle item`, (assert) => {
list.prepend(itemOne);
list.append(itemThree);
list.insertAfter(itemTwo, itemOne);

assert.equal(list.length, 3);
assert.equal(list.head, itemOne, 'head is itemOne');
assert.equal(list.tail, itemThree, 'tail is itemThree');
assert.equal(itemOne.prev, null, 'itemOne prev is null');
Expand All @@ -110,22 +153,16 @@ test('#insertAfter null reference item prepends the item', (assert) => {
let list = new LinkedList();
let item1 = new LinkedItem();
let item2 = new LinkedItem();
list.append(item1);
list.insertAfter(item2, null);

assert.equal(list.head, item2, 'item2 is appended');
assert.equal(list.tail, item1, 'item1 is at tail');
});

test('#insertBefore null reference item appends the item', (assert) => {
let list = new LinkedList();
let item1 = new LinkedItem();
let item2 = new LinkedItem();
list.append(item1);
list.insertBefore(item2, null);

assert.equal(list.tail, item2, 'item2 is appended');
assert.equal(list.head, item1, 'item1 is at head');
list.append(item2);
list.insertAfter(item1, null);

assert.equal(list.length, 2);
assert.equal(list.head, item1, 'item2 is appended');
assert.equal(list.tail, item2, 'item1 is at tail');
assert.equal(item1.next, item2, 'item1.next = item2');
assert.equal(item1.prev, null, 'item1.prev = null');
assert.equal(item2.prev, item1, 'item2.prev = item1');
assert.equal(item2.next, null, 'item2.next = null');
});

test(`#remove an only item`, (assert) => {
Expand All @@ -142,16 +179,16 @@ test(`#remove an only item`, (assert) => {
});

test(`#remove calls freeItem`, (assert) => {
let freedItem;
let freed = [];
let list = new LinkedList({
freeItem(item) {
freedItem = item;
freed.push(item);
}
});
let item = new LinkedItem();
list.append(item);
list.remove(item);
assert.equal(freedItem, item, 'item is freed');
assert.deepEqual(freed, [item]);
});

test(`#remove a first item`, (assert) => {
Expand All @@ -161,7 +198,8 @@ test(`#remove a first item`, (assert) => {
list.append(itemOne);
list.append(itemTwo);
list.remove(itemOne);
assert.equal(list.length, 1, 'length is one');

assert.equal(list.length, 1);
assert.equal(list.head, itemTwo, 'head is itemTwo');
assert.equal(list.tail, itemTwo, 'tail is itemTwo');
assert.equal(itemOne.prev, null, 'itemOne prev is null');
Expand All @@ -170,13 +208,14 @@ test(`#remove a first item`, (assert) => {
assert.equal(itemTwo.next, null, 'itemTwo next is null');
});

test(`#remove a second item`, (assert) => {
test(`#remove a last item`, (assert) => {
let list = new LinkedList();
let itemOne = new LinkedItem();
let itemTwo = new LinkedItem();
list.append(itemOne);
list.append(itemTwo);
list.remove(itemTwo);
assert.equal(list.length, 1);
assert.equal(list.head, itemOne, 'head is itemOne');
assert.equal(list.tail, itemOne, 'tail is itemOne');
assert.equal(itemOne.prev, null, 'itemOne prev is null');
Expand All @@ -194,6 +233,8 @@ test(`#remove a middle item`, (assert) => {
list.append(itemTwo);
list.append(itemThree);
list.remove(itemTwo);

assert.equal(list.length, 2);
assert.equal(list.head, itemOne, 'head is itemOne');
assert.equal(list.tail, itemThree, 'tail is itemThree');
assert.equal(itemOne.prev, null, 'itemOne prev is null');
Expand All @@ -204,6 +245,27 @@ test(`#remove a middle item`, (assert) => {
assert.equal(itemThree.next, null, 'itemThree next is null');
});

test(`#remove item that is not in the list is no-op`, (assert) => {
let list = new LinkedList();
let otherItem = new LinkedItem();

list.remove(otherItem);
assert.equal(list.length, 0);
});

test(`#remove throws if item is in another list`, (assert) => {
let list = new LinkedList();
let otherList = new LinkedList();
let otherItem = new LinkedItem();

otherList.append(otherItem);

assert.throws(
() => list.remove(otherItem),
/Cannot remove.*other list/
);
});

test(`#forEach iterates many`, (assert) => {
let list = new LinkedList();
let itemOne = new LinkedItem();
Expand Down

0 comments on commit f7a4ef2

Please sign in to comment.