Skip to content

Dequeue#6

Open
JSchatzman wants to merge 23 commits into
masterfrom
dequeue
Open

Dequeue#6
JSchatzman wants to merge 23 commits into
masterfrom
dequeue

Conversation

@JSchatzman
Copy link
Copy Markdown
Owner

No description provided.

Comment thread src/deque.py Outdated
return self.dll.shift()
self.head_node = self.dll.head_node
self.tail_node = self.dll.tail_node
self.length = self.dll.length
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the deque pop / dll shift method might be not be working as expected...

In [2]: dq = Deque([1, 2, 3, 4])

In [3]: dq.pop()
Out[3]: 1

In [4]: dq.pop()
---------------------------------------------------------------------------
ValueError: Linked list is already empty

while this works:

In [2]: dq = Deque([1, 2, 3, 4])

In [3]: dq.popleft()
Out[3]: 4

In [4]: dq.popleft()
Out[4]: 3

In [5]: dq.popleft()
Out[5]: 2

In [6]: dq.popleft()
Out[6]: 1

Comment thread src/deque.py Outdated
return self.dll.shift()
self.head_node = self.dll.head_node
self.tail_node = self.dll.tail_node
self.length = self.dll.length
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also:

In [7]: dq = Deque([1, 2])

In [8]: dq.popleft()
Out[8]: 2

In [9]: dq.popleft()
Out[9]: 1

In [10]: dq.pop()
Out[10]: 1

Comment thread src/deque.py Outdated

def peek(self):
"""Display but don't remove the contents of tail node."""
return self.dll.tail_node.contents
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peek should return none from an empty deque (see assignment page)

In [2]: dq = Deque()

In [3]: dq.peek()
---------------------------------------------------------------------------
AttributeError: 'NoneType' object has no attribute 'contents'

Comment thread src/deque.py Outdated

def peekleft(self):
"""Display but don't remove the contents of head node."""
return self.dll.head_node.contents
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peekleft should also return none from an empty deque (see assignment page)

In [4]: dq = Deque()

In [5]: dq.peekleft()
---------------------------------------------------------------------------
AttributeError: 'NoneType' object has no attribute 'contents'

Comment thread src/test_deque.py Outdated
one_deque = Deque(["one"])
empty_deque = Deque()
new_deque = Deque([1, 2, 3, 4, 5])
return one_deque, empty_deque, new_deque
Copy link
Copy Markdown

@bgarnaat bgarnaat Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this into three fixtures. Look into using parametrize to run each deque + expected output though a set of test functions if they should all behave the same (output val vs output error). Will make testing more clear / focused.

Comment thread src/test_deque.py Outdated

def test_deque_size_on_one(sample_deque):
"""Test for size on empty deque."""
assert sample_deque[0].size() == 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check Docstring (which deque is this test for)

Comment thread src/test_deque.py Outdated

def test_deque_peekleft_on_one(sample_deque):
"""Test for peekleft on empty deque."""
assert sample_deque[0].peekleft() == 'one'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check docstring (which deque is this for)

Comment thread src/test_deque.py Outdated

def test_deque_peek_on_one(sample_deque):
"""Test for peek on empty deque."""
assert sample_deque[0].peek() == 'one'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check docstring (which deque is this for)

Comment thread src/test_deque.py Outdated

def test_deque_pop_on_one(sample_deque):
"""Test for pop on empty deque."""
assert sample_deque[0].pop() == 'one'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check docstring (which deque is this for)

Comment thread src/test_deque.py Outdated
def test_deque_append_on_one(sample_deque):
"""Test for append on empty deque."""
sample_deque[0].append("hey")
assert sample_deque[0].tail_node.contents == "hey"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check docstring (which deque is this for)

Comment thread src/test_deque.py Outdated
def test_deque_appendleft_on_one(sample_deque):
"""Test for appendleft on empty deque."""
sample_deque[0].appendleft("hey")
assert sample_deque[0].head_node.contents == "hey"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check docstring (which deque is this for)

Comment thread src/deque.py

def append(self, contents):
"""Add value to the end of the deque."""
self.dll.append(contents)
Copy link
Copy Markdown

@bgarnaat bgarnaat Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something isn't hooked up right under the hood...

In [3]: dq = Deque()

In [4]: dq.append(1)

In [5]: dq.append(2)

In [6]: dq.dll.tail_node.previous_node
Out[6]: <__main__.Node at 0x7fdf70aea630>

In [7]: dq.dll.head_node.previous_node

In [8]: dq.dll.head_node.next_node

In [9]: dq.dll.tail_node.next_node

Copy link
Copy Markdown

@bgarnaat bgarnaat Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either head or tail should have a next node.

Comment thread src/test_deque.py
def test_deque_empty_init(empty_deque):
"""Test for empty deque init."""
assert empty_deque.dll.head_node is None
assert empty_deque.dll.tail_node is None
Copy link
Copy Markdown

@bgarnaat bgarnaat Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this into two tests. One that confirms head_node and one that confirms tail_node.

Comment thread src/test_deque.py
def test_deque_one_init(single_deque):
"""Test for one item deque init."""
assert single_deque.dll.head_node.contents == "one"
assert single_deque.dll.tail_node.contents == "one"
Copy link
Copy Markdown

@bgarnaat bgarnaat Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this into two tests. One that confirms head_node and one that confirms tail_node.

Comment thread src/test_deque.py
def test_deque_init(sample_deque):
"""Test for new deque init."""
assert sample_deque.dll.head_node.contents == 5
assert sample_deque.dll.tail_node.contents == 1
Copy link
Copy Markdown

@bgarnaat bgarnaat Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this into two tests. One that confirms head_node and one that confirms tail_node.

Comment thread src/test_deque.py Outdated

def test_deque_size_increase(sample_deque):
"""Test that size increases after appending."""
testing_deque = sample_deque
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you binding sample_deque to testing_deque. The purpose of fixtures is to have an object ready to use for a single test. Remove this line and use sample_deque like the rest of the tests you run.

Comment thread src/test_deque.py Outdated

def test_deque_size_decrease(sample_deque):
"""Test that size decreases after appending."""
testing_deque = sample_deque
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants