dibs: implement simple-linked-list by laurauzcategui · Pull Request #385

dibs: implement simple-linked-list by laurauzcategui · Pull Request #385 · exercism/python

@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.

linked-list is a quite similar exercise #212 which might help you while working on this new exercise. There you can see how to add the exercise to the config.json.
For possible test cases you could have a look at other tracks like C# or ruby. We use flake8 as linter (details). You might need to run that, to fix some of your code style problems.

LauraU added 2 commits

October 27, 2016 11:01

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?

Pull requests do not have to be merged and accepted; as long as they've been opened between the very start of October 1 and the very end of October 31, they count towards a free T-shirt. – source

So, @kytrinyx there is no rush to merge this today and I will review it probably this week.

@laurauzcategui The conflict with the config.json can be resolved with a rebase of upstream/master.

Hi @behrtam, @kytrinx

Thanks. Simple-linked-list is not ready yet, but binary-search-tree I believe it is :-)

And as @behrtam said, there is no rush for Hacktoberfest :) I already received the email from digitalOcean that my PRs are valid :)

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 "push-down stacks (also called a LIFO stack; Last In, First Out)".
Sadly we don't have a canonical-data.json for this problem, that would make it more clear. As a result while the ruby track tests exactly for this (test_items_are_stacked), the C# track does it the other way around (Two_item_list_second_value).

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. push should add to the head, pop should take from the head, peek should give you the value of the head.

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.

Choose a reason for hiding this comment

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

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):

Choose a reason for hiding this comment

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

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):

Choose a reason for hiding this comment

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

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:

Choose a reason for hiding this comment

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

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):

Choose a reason for hiding this comment

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

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=[]):

Choose a reason for hiding this comment

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

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 = []

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

No need for undescriptive short variable names, there is enough space for element = self....

We have decided to use stub files for all exercises (#415) so you would need to add this here as well.

@behrtam I'd recommend documenting this in the README.

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.

Has this been implemented or just closed due to inactivity?

Choose a reason for hiding this comment

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

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/
Keep in mind that in this case you don't want to fetch from origin but from upstream.

So, for me this looks like this:

➜  python git:(master) git remote -v
origin	git@github.com:behrtam/python.git (fetch)
origin	git@github.com:behrtam/python.git (push)
upstream	https://github.com/exercism/python.git (fetch)
upstream	https://github.com/exercism/python.git (push)

➜  python git:(master) git fetch upstream pull/385/head:carry-on-pr-385
remote: Counting objects: 80, done.
remote: Total 80 (delta 21), reused 21 (delta 21), pack-reused 59
Unpacking objects: 100% (80/80), done.
From https://github.com/exercism/python
 * [new ref]         refs/pull/385/head -> carry-on-pr-385

➜  python git:(master) git checkout carry-on-pr-385
Switched to branch 'carry-on-pr-385'

➜  python git:(carry-on-pr-385) python3 test/check-exercises.py simple-linked-list
.....
----------------------------------------------------------------------
Ran 5 tests in 0.000s

OK

➜  python git:(carry-on-pr-385)

If you have any questions or need any help. Just ask.