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

Rock - Aeron Roemer #1

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

Conversation

AeronRoemer
Copy link

I forked before the changes & used my own linked list from the previous assignment.

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.

Nice work Aeron, you really got into things quickly and you hit the main learning goals here. I made some suggestions and take a look at my feedback and let me know what questions you have.

Comment on lines +12 to +13
if (this.front == 0 && (this.rear == this.store.size - 1)){
return false

Choose a reason for hiding this comment

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

This if condition will prevent the rear from wrapping around the buffer, which is the behavior you want.

I think it should instead be:

Suggested change
if (this.front == 0 && (this.rear == this.store.size - 1)){
return false
if ((this.rear +1) % this.store.size == this.front){
return false

this.rear = (this.rear + 1) % this.size
this.store[this.rear] = element
}

}

dequeue() {

Choose a reason for hiding this comment

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

👍

}

front() {
throw new Error("This method has not been implemented!");
frontItem() {

Choose a reason for hiding this comment

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

👍

}

size() {
throw new Error("This method has not been implemented!");
sizeOfQ() {

Choose a reason for hiding this comment

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

👍 , although since this size method is a member of the Queue instance, the "Q" is kind of assumed.

}

pop() {
throw new Error("This method has not been implemented!");
if (!this.store.head) return null;

Choose a reason for hiding this comment

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

You could also use the linked list empty method.

throw new Error("This method has not been implemented!");
if (!this.store.head) return true;

if (this.store.head.next) return false;

Choose a reason for hiding this comment

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

This could give a false empty when the Stack has 1 element. You can just return false if the prior if statement doesn't exit.

Suggested change
if (this.store.head.next) return false;
return false;

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