Skip to content

Stack#2

Open
julienawilson wants to merge 43 commits intomasterfrom
stack
Open

Stack#2
julienawilson wants to merge 43 commits intomasterfrom
stack

Conversation

@julienawilson
Copy link
Collaborator

pull request for the Stack Data Structure.

Contains the Stack module and testing module.


def __init__(self, iterable=None):
"""Instantiate stack."""
self.linked_list = LinkedList(iterable)

Choose a reason for hiding this comment

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

Good use of composition!

"""Instantiate stack."""
self.linked_list = LinkedList(iterable)
self.length = self.linked_list.length
self.head_node = self.linked_list.head_node

Choose a reason for hiding this comment

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

However, since you have all of the linked lists's functionality here, do you really need to assign a head_node variable? What purpose is it serving?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we want to check the value of the head node, we need to assign it in the Stack, correct?

I suppose we could get it using my_stack.linked_list.headnode, but isnt that kind of ugly?

src/stack.py Outdated
"""Remove and return the current head node."""
old_head_node = self.linked_list.pop()
self.head_node = self.linked_list.head_node
return old_head_node

Choose a reason for hiding this comment

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

Similar to my comments on your linked list, your Stack should just return the value on top of the stack, not the whole Node.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our linked_list pop now returns a value, which means the stack does too. Are you objecting to the name?

"""Test for Stack init."""
from stack import Stack
new_stack = Stack()
assert new_stack.length == 0

Choose a reason for hiding this comment

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

You guys need to fix your tests so there's only one assert statement per test function. The reason for this is so you can get a more granular understanding of what cases/scenarios fail and which don't.

A big problem is that if one assertion is false, the function exits and the rest of the tests are not evaluated, so you are denied more information about the success/failure of your tests. Separate those concerns. Be explicit.

And once again, use fixtures and parametrization. It can save you from so much of this messy multi-line test function malarkey.

@@ -0,0 +1,15 @@
"""The setup for Mailroom distribution."""
Copy link

Choose a reason for hiding this comment

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

The mailroom? I just wanted my fancy data structures!

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.

4 participants