LinkedList src and spec file... Any input GREATLY APPRECIATED... Thanks in advance... #88
Conversation
|
|
||
|
|
||
| it('exists', () => { | ||
|
|
|
|
||
| it('returns head node', () => { | ||
| const linkedList = new LinkedList() | ||
| linkedList.insert('apple') |
There was a problem hiding this comment.
you used the before hook... that should do this before each test... why are you still doing it for every test?
There was a problem hiding this comment.
it didn't work... lol... need to take that out...
| linkedList.insert('banana') | ||
|
|
||
| expect(linkedList.getTailNode().data).to.equal('banana') | ||
|
|
There was a problem hiding this comment.
Everywhere, I'm not going to comment them all. It almost doesn't matter which way you pick, just be maximum consistent before submitting PR
| } | ||
|
|
||
| getHeadNode(){ | ||
| if (!this) { |
There was a problem hiding this comment.
change this to a ternary
!this ? null : this.head
| } | ||
|
|
||
| getTailNode(){ | ||
| if (!this) { |
|
|
||
| contains(string){ | ||
| let current = this.head | ||
| if (this.head.data == string) { |
There was a problem hiding this comment.
If I have 10 things in my list will this still be right? seems like it's checking the first two levels....
|
|
||
| insert(data){ | ||
| const current = new Node(data) | ||
| this.size ++ |
There was a problem hiding this comment.
maybe increment size at the end in case something fails?
| this.head = newHead | ||
| } | ||
| isEmpty() { | ||
| if (this.head === null && this.tail === null && this.size === 0) { |
There was a problem hiding this comment.
returning true/false is a code smell. You should always look at how you can refactor.
if you just do return (this.head === null && this.size === 0) that will return t/f
There was a problem hiding this comment.
I did that but in one of my tests, I found that one of the nodes still existed in the tail of my linked list somehow?? Weird as hell because I thought that if you removed the head node without reassigning 'this.head' that the whole list would magically disappear...
| } | ||
|
|
||
| getNext() { | ||
| if (this.next) { |
There was a problem hiding this comment.
You're the best @rachel-ftw... Thanks for your detailed feedback, the ninja training is strong in this one... lol
spec/linkedList.js
Outdated
| let linkedList = new LinkedList() | ||
| linkedList.insert('apple') | ||
| linkedList.insert('banana') | ||
| }) |
There was a problem hiding this comment.
It seems like you want this function to be run before each test. You could use beforeEach for this (before only runs once). See https://mochajs.org/#hooks.
Also, the way linkedList is declared on line 8 means that this variable is only defined within the block on lines 7 through 11.
But all the tests need access to linkedList so this must be fixed.
One way to fix this is to declare linkedList on line 6 (global scope) without assigning it a value and then assign a value to linkedList within the beforeEach like you are already doing.
So, line 6 would become let linkedList and line 8 would be linkedList = new LinkedList().
Then you could remove these three lines in each of the tests,
const linkedList = new LinkedList()
linkedList.insert('apple')
linkedList.insert('banana')
…ith feedback from PR
|
|
||
| expect(linkedList.tail).to.equal(null) && | ||
| expect(linkedList.head).to.equal(null) && | ||
| expect(linkedList.size).to.equal(0) |
|
|
||
| context('isEmpty', () => { | ||
| const linkedList = new LinkedList() | ||
| linkedList.isEmpty() |
|
|
||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Good job on these linked list tests! There are some blank lines which could be removed to make it look a little cleaner.
| getHeadNode(){ | ||
| if (!this) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Lines 12 to 14 can be removed. If you put a console.log inside the if you'll find that this code is never run, even when there is no head node.
| getTailNode(){ | ||
| if (!this) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Lines 19 to 21 can be removed. See above.
| if (current.data !== string && current.next == null) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
This method/function called contains does not meet the spec because it only returns true if it finds the data in one of the first two nodes.
For example, if your test contained,
linkedList.insert('apple')
linkedList.insert('banana')
linkedList.insert('cranberry')
expect(linkedList.contains("apple")).to.equal(true)
expect(linkedList.contains("banana")).to.equal(true)
expect(linkedList.contains("cranberry")).to.equal(true)
the "apple" and "banana" tests will pass but the "cranberry" test will fail.
I suggest you start over from scratch with this function. I suggest using a whiteboard or paper or pseudocode to figure out an algorithm which will find out if a piece of data is in the linked list.
|
|
||
| expect(linkedList.find("banana").data).to.equal('banana') | ||
|
|
||
| }) |
There was a problem hiding this comment.
It would be good to include a test for the -1 case as well.
3 participants
No description provided.