Skip to content

linked-list: Implemented new exercise #212

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

Merged
merged 7 commits into from
Oct 28, 2016

Conversation

behrtam
Copy link
Contributor

@behrtam behrtam commented Nov 6, 2015

I implemented the x-common/linked-list exercise (README.md). While Python has a collections.deque since 2.4 I still think you could learn a lot from implementing this.

I added a skeleton file that already has a class Node to help starting in the right direction.

@kytrinyx
Copy link
Member

kytrinyx commented Nov 6, 2015

Thanks @behrtam, this is much appreciated.

I'm going to leave this for @Dog to review/merge.

@Dog
Copy link
Contributor

Dog commented Nov 7, 2015

I'll take a deeper look at it tonight, but these were my notes looking at the problem.

Notes regarding problem as a whole

  • The ReadMe does not prohibit structures implemented by the standard library
  • I wish the exercise didn't use the abbreviated name of double ended queue. It could be confused with dequeue later on. Dequeue is commonly used to refer to removing an element from a FIFO queue.
  • I wish that the ReadMe mentioned sentinel nodes for extra reading.

Notes regarding your implementation

  • It looks like your code can't handle a call attempting to remove an element from an empty queue. It would also be nice to test this case.
  • It'd be ideal not to use the variable name next, only because of overwriting the built in function. A better name isn't immediately coming to mind. Its very minor though. Its really if we can come up with a synonym, it'd be preferable.

Extra Credit Ideas

What I think would be very cool to do for this problem is to have users implement a couple magic methods like __iter__ in python. I'm not sure how to add the extra language specific information for it, but this means they could then iterate their structure just like a list.

for x in my_deque:
    do_something(x)

It would also work with built ins:

iterator = iter(my_deque)
first = next(iterator)
second = next(iterator)

It could also be interesting to make them implement __len__, and keep track of the size of their structure.

my_deque = Deque()
my_deque.push(1)
my_deque.push(2)
size = len(my_deque)
assert size == 2

Some other magic methods to consider are __getitem__, __setitem__, and __reversed__

@behrtam
Copy link
Contributor Author

behrtam commented Nov 7, 2015

Comments regarding the problem as a whole

  • I don't have a solution to prohibit structures implemented by the standard library. My best idea was to provide a skeleton file with a class Node implementation to get the user on the right track.
  • I was also confused reading the name Deque in a problem with that name linked-list. But I didn't want to differ from the readme, so I stayed with the name.
  • We could provide a link for extra reading about sentinel nodes in the skeleton file if you know something good.

Comments regarding my implementation

  • You are right my code can't handle removing an element from an empty queue. That didn't happen by accident as I didn't want to conflict with the readme.

    To keep your implementation simple, the tests will not cover error conditions. Specifically: pop or shift will never be called on an empty Deque.

    So I mad sure my implementation can't handle a remove if it's empty, so my tests don't accidentally check for that.

  • We could replace prev with left and next with right.

    It'd be ideal not to use the variable name next, only because of overwriting the built in function.

  • Do you think it would have been better to implement it as a ring with only a head as reference?

Comments regarding your extra credit ideas

I really like your idea. We could totally make this about the magic methods, by providing already the method signatures for the magic methods (we can't put them in the readme) and maybe push/pop implementation.
Would also fix the problem from the top of using structures implemented by the standard library.

@Dog
Copy link
Contributor

Dog commented Nov 7, 2015

Comments regarding the problem as a whole

  • There are a few exercises that the ReadMe states you should not use a standard library function if it exists in a language. It isn't something that we enforce. Its kind of a cue that it is an intellectual exercise, and this is a structure commonly used in a lot of problems (and therefore really handy to be familiar with)
  • Yeah, I understand that. I wrote it down here first because I wasn't quite ready to suggest a change for every language in x-common. I wanted to collect my thoughts a bit more, but I thought @kytrinyx may comment on it before I came back to this request.
  • I did a quick search for a good source, and I'm having trouble finding anything I really like. I'll keep looking.

Comments regarding my implementation

  • Okay, I missed that.
  • I think it'll be okay to use next in this context since its really an intellectual exercise.

Comments regarding your extra credit ideas

@kytrinyx Do you have an idea how we might be able to do this in the system currently?

@behrtam behrtam changed the title Implemented linked-list exercise from x-common linked-list: Implemented new exercise Nov 8, 2015
@kytrinyx
Copy link
Member

kytrinyx commented Nov 8, 2015

Sorry, I'm been traveling and dropped offline for a couple of days. Yeah, I think we should fix the README, and then adjust any implementations that need it based on the more accurate and useful README.

For the extra credit stuff, we've just been putting it with pending tests and a comment saying extra credit. What I'd really like to do is have a file that can be included into the README on a per-exercise basis, so that we could describe the extra credit and have it just show up as a part of the problem description.

@behrtam
Copy link
Contributor Author

behrtam commented Oct 27, 2016

So, apparently all the Dequeue confusion got fixed (commit).

Maybe we could add the additional magic methods in the same skipped fashion like we discussed for the clockexercise (see #373).

@behrtam behrtam merged commit cfde6bf into exercism:master Oct 28, 2016
@behrtam behrtam deleted the implement-linked-list branch March 22, 2017 15:17
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.

3 participants