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

Linked List: adding 'delete' to required methods #1559

Closed
wants to merge 1 commit into from

Conversation

jocelo
Copy link

@jocelo jocelo commented Jul 23, 2019

Adding 'delete' to the list of methods that the object should have; in the assertions the delete method is expected to be part of the object.

adding 'delete' to the list of methods the method should have: test suite is expecting the delete method implemented in some assertions.
@petertseng
Copy link
Member

There are a few possible meanings for delete, so first let me list them out so we can get on same page

  • delete: Given a way to refer to a node, delete that node from the list containing it, in O(1) time.
  • delete: Given a value and a list, search the list for a node containing that value in O(n) time, then delete it (as above).
  • delete: Given an index and a list, advance that many positions into the list in O(n) time, and delete it (as above)

When I read a description of "remove any node by value", it seems to me to imply the second possibility for delete I mentioned above. If that is as intended, then great. If not, let's decide which other possibility is intended and have a description that designates that particular possibility.

A note for tracks implementing this exercise (noting that we have no canonical-data.json) that if deletion is added, it will probably be wise to test deletion from the head of the list, from the tail of the list, and from the middle. Take this into account when deciding whether to add delete.

I will make my recommendation in a next comment.

@petertseng
Copy link
Member

Personally I'll recommend against adding any of the three above possibilites of delete.

For "delete, given a way to refer to a node", I recommend against with preference strength 3/10. Note that this is the first time we allow a client of this list to refer to a node. In all other existing operations, they only have to be able to refer to a list and can remain completely ignorant that any nodes exist at all. It's not inherently bad to give the client more knowledge, but it will require more work on the part of students + mentors to make sure the interface thus provided makes sense.

For "delete, given a value and a list" and "delete, given an index and a list" I will recommend against with preference strength 1/10 because it "feels bad" to add an O(n) operation to this list. There's nothing wrong with it since these are possibly things that someone would want to be able to do to a list, so I did not give it any higher strength. After all, if the client uses these variants of delete, knowing full well they are O(n) and that is what they want, then I can't deny them what they want.

@kytrinyx
Copy link
Member

Hello. Thank you for this PR - it's really appreciated.

This repo is currently on "lock-down" while we think through a few things. As such, I'm going to put this PR on hold for now, and we will come back to once we've worked through the issues with this repo that we need to address. You can read more here.

Sorry that you've hit us at this momentarily awkward juncture, and thank you again for the contribution.

@kytrinyx kytrinyx added the hold label Aug 12, 2019
@jocelo
Copy link
Author

jocelo commented Aug 13, 2019

Hi @petertseng thanks for your in-dept comment about this. From my short experience with this project, I have come to know that more "advanced" topics are not part of any discussion when mentoring students (advanced as in time/space complexity, recursion, best practices). Other mentors have always pointed out that we should maintain a non-strict posture when helping students. Your comments about algorithm complexity makes a lot of sense and I like the idea that this topics are in the discussion board, even if they are not part on the current state of the project. I'll be more than happy to continue contributing to this project as much as I can!

@kytrinyx don't worry about it and many thanks for letting me know about the lock down.

This is an awesome project and I enjoy the community that's forming around it.

Base automatically changed from master to main January 27, 2021 15:31
@ErikSchierboom
Copy link
Member

@jocelo Are you still interested in working on this PR?

@jocelo
Copy link
Author

jocelo commented Jan 12, 2022

@ErikSchierboom yeah, still interested... re-reading comments from Peter and myself above looks like the PR is no longer valid, so I think it should be closed, please let me know if you have any additional comments, otherwise I'll close it.
Thanks!

@ErikSchierboom
Copy link
Member

I agree. I'll close. Thanks for creating the PR!

@jocelo
Copy link
Author

jocelo commented Jan 12, 2022

Back at you!

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