Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nicole Washington:Maple #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

N-Washington
Copy link

Linked List

Copy link

@kyra-patton kyra-patton left a comment

Choose a reason for hiding this comment

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

⚠️ Overall good work, Nicole, however there are some comments you should take a look at. It may have been because of the work you lost and the rush to complete that work again. Look over my comments and feel free to reach out with your questions.

🟡

# Time Complexity: ?
# Space Complexity: ?
# Time Complexity: constant, no traversing needed
# Space Complexity: constant, no new data structures
def get_first(self):

Choose a reason for hiding this comment

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

# Time Complexity: ?
# Space Complexity: ?
# Time Complexity: constant
# Space Complexity: constant
def add_first(self, value):

Choose a reason for hiding this comment

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

# Time Complexity: ?
# Space Complexity: ?
# Time Complexity: linear
# Space Complexity: constant
def search(self, value):

Choose a reason for hiding this comment

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

if self.head is None:
return False
current_node = self.head
target_node = Node(value)

Choose a reason for hiding this comment

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

You can create a new node for the target, but it's not necessary. See my suggestion on line 45 ⬇️


# traverse list til reach target node or end of list
while current_node:
if current_node.value == target_node.value:

Choose a reason for hiding this comment

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

current_node.value is an integer, so instead of creating a new node target_node that has value value, you can just compare the current node's value directly to the passed in value.

Suggested change
if current_node.value == target_node.value:
if current_node.value == value:

def add_last(self, value):
pass
if self.head is None:
self.head = value

Choose a reason for hiding this comment

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

🤔 self.head should be equal to a node object, not an integer like value

Comment on lines +122 to +124
new_tail_node = prev_node
current_node.next = new_tail_node
new_tail_node.next = None

Choose a reason for hiding this comment

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

Here, after creating the new_tail_node on line 115, you are overwriting it with prev_node which, once the while loop is finished executing, should be the current tail node.

Once the while loop is finished executing current_node is going to be the node after the the current tail, aka None.

Instead, try pointing the current tail (prev_node) to new_tail_node

while current_node:
if current_node.value > max_node.value:
max_node = current_node
current_node = current_node.next

Choose a reason for hiding this comment

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

🤔You're creating an infinite loop here since you only reassign current_node to the next node in the list if current_node.value > max_node.value

Comment on lines +106 to +107
# Time Complexity: linear
# Space Complexity: constant

Choose a reason for hiding this comment

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

💫 Correct on the time and space complexity, however see my comments below ⬇️

next_node = current_node.next
target_value = value
if current_node.value == target_value:
current_node = current_node.next

Choose a reason for hiding this comment

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

This makes current_node equal to what was the second node in the list, but it doesn't redirect self.head to point at the second node in the list

Suggested change
current_node = current_node.next
self.head = current_node.next

# Time Complexity: ?
# Space Complexity: ?
# Time Complexity: linear
# Space Complexity: linear
def visit(self):

Choose a reason for hiding this comment

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

# Time Complexity: ?
# Space Complexity: ?
# Time Complexity: linear
# Space Complexity: constant
def reverse(self):

Choose a reason for hiding this comment

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

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.

2 participants