Conversation
jrobcodes
left a comment
jrobcodes
left a comment
There was a problem hiding this comment.
There are a number of places in your code where the indentation is not consistent. Please consistently use indentation to increase readability and maintainability.
|
|
||
| expect(() => doubleLinkedList.insert('foo')) | ||
| .to.alter(() => doubleLinkedList.count, { from: 0, to: 1 }) | ||
| expect(doubleLinkedList.getHeadNode().data) |
There was a problem hiding this comment.
Instead of relying on the internal data representation in Node, consider adding an accessor method:
doubleLinkedList.getHeadNode().getData()If you later choose to change how data is represented in the node, since you have programmed to the interface, instead of the implementation, your code (both in this test and in the list implementation) will not need to change.
There was a problem hiding this comment.
hmm, can I get an example?
There was a problem hiding this comment.
class Node {
getData() {
return this.data
}
}Everywhere you currently use node.data would need to be updated to node.getData(). Now, regardless of any change you make to the internal structure of a Node, so long as you preserve the getData functionality, your code will not break.
There was a problem hiding this comment.
More on this: let's say I change Node's data field to be named lkhsklkhdsfglkh. I would then change getData to return that field. Anything in our codebase that was referencing node.data would break. Any where that was referencing getData would not.
There was a problem hiding this comment.
ahhhh, I see. That makes a lot more sense now.
| doubleLinkedList.insertFirst('foo') | ||
| doubleLinkedList.insertFirst('boo') | ||
|
|
||
| expect(doubleLinkedList.contains('foo')) |
There was a problem hiding this comment.
I try to create tests that test only one assertion - the true and false cases are two different tests that are not necessarily related. If you somehow manage to "break" the true path, this test would count as failed, even though the second assertion may not be failing... Isolating assertions in individual tests helps you granularly test, and explicitly identify broken functionality with the test suite.
| doubleLinkedList.insertFirst('boo') | ||
| doubleLinkedList.insert('shoo') | ||
|
|
||
| expect(doubleLinkedList.clear()) |
There was a problem hiding this comment.
This is not a meaningful test - a function without an explicit return statement returns undefined in JavaScript. This test will always pass, even if the clear() function is broken. You have a number of tests like this that should be removed (and perhaps replaced by tests that will fail if the function is broken).
|
|
||
| find(item) { | ||
| let currentNode = this.head | ||
| while (currentNode) { |
There was a problem hiding this comment.
Indentation should be consistently left aligned at the same level of a given block.
| this.tail = newNode | ||
| this.count++ | ||
|
|
||
| return newNode |
There was a problem hiding this comment.
Should we be returning a Node from the insert function?
There was a problem hiding this comment.
should we have any return?
| const newNode = new Node(data) | ||
|
|
||
| if (this.find(item) === this.tail { | ||
| newNode.prev = this.tail |
| } else { | ||
| let currentNode = this.head | ||
| while (currentNode.next !== newNode.next) { | ||
| currentNode = currentNode.next |
There was a problem hiding this comment.
currentNode = currentNode.nextis in the while block , so should be indented under while:
} else {
let currentNode = this.head
while( currentNode.next !== newNode.next ) {
currentNode = currentNode.next
}
}| if (this.count === 1) { | ||
| this.clear() | ||
| } else { | ||
| let currentNode = this.getHeadNode() |
There was a problem hiding this comment.
I don't understand this remove code? Couldn't you just use tail's prev to do this?
const newTail = this.tail.prev
this.tail.prev = null
newTail.next = null
this.tail = newTail| this.clear() | ||
| } else { | ||
| this.head = this.getHeadNode().next | ||
| this.count-- |
There was a problem hiding this comment.
You would also need to reset the new head's prev reference to null.
| } | ||
|
|
||
| isEmpty() { | ||
| if(!this.head){ |
There was a problem hiding this comment.
I prefer explicit comparisons, instead of relying on "falsiness" of null/undefined:
return this.head === undefinedAlso, isEmpty should be returning a boolean true or false. Your implementation returns "truthy" or "falsey".
There was a problem hiding this comment.
can you tell me more about the difference?
There was a problem hiding this comment.
I suggest you check out https://github.com/getify/You-Dont-Know-JS/blob/master/types%20%26%20grammar/ch4.md to understand coercion and its implications for truthyness and falsyness.
2 participants
No description provided.