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

Ports - Ari #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ports - Ari #28

wants to merge 2 commits into from

Conversation

aribray
Copy link

@aribray aribray commented Aug 28, 2019

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Not your best work. A few comments.

  1. It seems your new IDE is messing up your indentation and causing you to check in XML files to git. Something to work on.
  2. Look for ways to reuse methods, some of them can pay off here.

You hit most of the learning goals, but definitely had some problems. Take a look at my comments and let me know what questions you have.

# Space Complexity
def delete(value)
raise NotImplementedError
visit_array = []

Choose a reason for hiding this comment

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

You have some weird indentation here.

@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="UTF-8"?>

Choose a reason for hiding this comment

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

What is all this? Maybe put .idea folder in .gitignore.

# Time Complexity:
# Space Complexity
# Time Complexity: O(1) (constant), because the length of the list doesn't matter
# Space Complexity: O(n), where n is the length of the list

Choose a reason for hiding this comment

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

But the method itself doesn't add more space to the list, it only adds 1 element.

def search(value)
raise NotImplementedError
node = @head
return false if !node.next

Choose a reason for hiding this comment

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

What if @Head is nil?
What if the value is in the head and the list only has 1 element?

raise NotImplementedError
current = @head

return 0 if current.nil?

Choose a reason for hiding this comment

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

Do you need this return?

current = current.next
count += 1
end
current.data

Choose a reason for hiding this comment

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

What if the index is past the end of the list?

previous_node = nil


while current_node

Choose a reason for hiding this comment

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

👍

else
before.next = current.next
end
return "deleted"

Choose a reason for hiding this comment

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

??

# method to delete the first node found with specified value
# Time Complexity: O(n)
# Space Complexity: O(1)
def delete(value)

Choose a reason for hiding this comment

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

This isn't working.

def find_nth_from_end(n)
raise NotImplementedError
selected_index = length - n - 1

Choose a reason for hiding this comment

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

Could you have also used the method to get an element at an index?

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