dibs: implement simple-linked-list #385
Conversation
|
@kytrinyx could you please review? I'm new to exercism and I was trying to figure it out if this was the right way to get started. |
|
|
|
Thanks @behrtam 👍 Fixed the issues with flake8. Will be adding test cases and other helpers with progress of the days. Cheers, |
|
@behrtam will you have time to review (and potentially merge) before the end of Hacktoberfest (Monday, end of day)? |
|
@laurauzcategui there's a conflict in the config.json—would you mind fixing it? |
So, @kytrinyx there is no rush to merge this today and I will review it probably this week. @laurauzcategui The conflict with the |
|
Beside some details my first impression is good but we do have a more abstract problem. Normally singly linked lists insert and delete at the beginning of the list therefor the complexity is O(1). This is also the behaviour mentioned in the readme of the problem Until we have canonical test data and we found a way to harmonize all the tracks, I would go with the readme and change your behaviour. |
|
Hi @behrtam updated based on your recommendations. Looking for a new review :) Cheers, |
|
Great! I should find some time to review that in the next days. |
|
This issue has been automatically marked as `on hiatus because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@behrtam Did this one fall between the cracks? Would you have time to give it a look? |
|
Oh, I'm sorry. Should finde some time on Sunday. |
|
Awesome, thanks! 🌻 |
There was a problem hiding this comment.
Okay, finally I found some time ...
We have decided to use stub files for all exercises (#415) so you would need to add this here as well.
Right now you have quite complex tests, one test case for each functionality. Instead I would like to see tests that are more concise (in most cases one assert per test) which work in small incremental steps. Maybe the ruby tests can be of some help: https://github.com/exercism/xruby/blob/master/exercises/simple-linked-list/simple_linked_list_test.rb
You might have to rebase to update this PR with some of the changes that happened in the meantime.
| self.head = head | ||
| self.size = 0 | ||
|
|
||
| def push(self, new_element): |
There was a problem hiding this comment.
The naming needs to be improved. Right now one can push values, than the parameter should be named accordingly or the method should accept Elements instead.
| else: | ||
| return self.head.value | ||
|
|
||
| def reverse(self): |
There was a problem hiding this comment.
The convention is that reverse/sort/etc work change the actual instance, while reversed/sorted/etc would return a new changed instance leaving the other instance unchanged.
|
|
||
| def reverse(self): | ||
| new_list = LinkedList() | ||
| if self.size > 0: |
There was a problem hiding this comment.
You don't need to check size and the current element, one check is sufficient. It makes the code more understandable:
def reversed(self):
new_list = LinkedList()
current = self.head
while current:
new_list.push(current.value)
current = current.next
return new_list|
|
||
|
|
||
| class LinkedList(object): | ||
| def __init__(self, head=None): |
There was a problem hiding this comment.
The head argument is never used. Maybe it could be used for the from array list creation instead.
| arr.append(current.value) | ||
| return arr | ||
|
|
||
| def from_array(self, arr=[]): |
There was a problem hiding this comment.
I would either implement the from_array initializer using the __init__(self, array=None) method or a @classmethod (see: https://stackoverflow.com/a/682545).
| return new_list | ||
|
|
||
| def to_array(self): | ||
| arr = [] |
There was a problem hiding this comment.
This method also doesn't need two checks for the same thing. There is also no need in cutting variable names short.
def to_array(self):
array = []
current = self.head
while current:
array.append(current.value)
current = current.next
return array| if self.empty(): | ||
| return None | ||
| else: | ||
| e = self.head |
There was a problem hiding this comment.
No need for undescriptive short variable names, there is enough space for element = self....
|
This issue has been automatically marked as |
|
Has this been implemented or just closed due to inactivity? |
|
This is closed for inactivity. It doesn't look like anyone else has implemented in the mean time (it would show up in this list on master: https://github.com/exercism/python/tree/master/exercises) @behrtam reviewed it and asked for some changes. If you want to carry the changes and implement the fixes that @behrtam suggested that would be awesome. Check out Jess Frazelle's blog post, The Art of Closing, especially point 4 "The Carry" if you're interested in helping out with this. https://blog.jessfraz.com/post/the-art-of-closing/ |
There was a problem hiding this comment.
I'd like to try implementing these changes, I'm somewhat of a newbie to git and exercism. I've done a few python exercism exercises on my own but I want to contribute to this one with alittle help. The first question is how do I go about seeing the files that need changes? Second, then after making any edits, how do I recommit them? Is it the process from this link? https://github.com/exercism/docs/blob/master/you-can-help/implement-an-exercise-from-specification.md
|
Step 1) Fork this repository and clone it locally. If you don't know how to do this take a look at this guide: https://help.github.com/articles/fork-a-repo/ Step 2) Add the upstream repo as remote. Details on how to do this are here: https://help.github.com/articles/configuring-a-remote-for-a-fork/ Step 3) Fetch the PR into a local branch, to continue the work and later create a new PR. The steps on how to checkout a PR locally can be found here: https://help.github.com/articles/checking-out-pull-requests-locally/ So, for me this looks like this: If you have any questions or need any help. Just ask. |
|
Thanks for information, I did steps 1 and 2. However, the checkout didn't work. The images below said local changes would be overwritten by checkout.
Any thoughts? Other questions I have is I'm not entirely sure how to setup testing for this exercise. I looked at these links below and want to know if I'm in the right direction. https://github.com/exercism/exercism.io/blob/master/CONTRIBUTING.md |
Opened also: #384