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

Fix LinkedList removeAt() #98

Closed
wants to merge 2 commits into from
Closed

Fix LinkedList removeAt() #98

wants to merge 2 commits into from

Conversation

YahavArm
Copy link
Contributor

1.added base case for removeAt(1)
2.switched assigment on line #138 where previous and current where pointing to the same node

1.added base case for removeAt(1)
2.switched assigment on line #138 where previous and current where pointing to the same node
Copy link
Member

@ashokdey ashokdey left a comment

Choose a reason for hiding this comment

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

Hello @TheKingSombrero, thanks for the PR and it's good to see you took an interest. However, there are a few things that you have to see:

  • ESLint rules should not break
  • Remove methods should always return a node (and the next should be null)
  • Please go through the issue details here: removeAt function in linked-list #83
  • The removal method should work as an array

Thanks!

}

previous.next = address.next;
console.log(address.next)
Copy link
Member

Choose a reason for hiding this comment

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

No console.log() in the final push to master

passes eslint and matches and enforced strict equality
for some reason it wasn't running in node with it

if (index >= this.length()) {
if (index === 0) {
let returnNode=this.head;

Choose a reason for hiding this comment

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

why this?

let returnNode=this.head;
this.head=this.head.next;
this.size -= 1;
return returnNode;

Choose a reason for hiding this comment

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

return this.head from here

Copy link
Member

Choose a reason for hiding this comment

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

  if (!this.head) {
      this.head = node;
      this.tail = node;
      return node;
    }

If you do not make it null it will contain the full list

this.size -= 1;
return returnNode;
}
if (index >= this.size) {

Choose a reason for hiding this comment

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

in linked list we don't keep size

previous = address;
address = address.next;

Choose a reason for hiding this comment

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

better name

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

Successfully merging this pull request may close these issues.

3 participants