Revision e71ef8b8-abd5-4e66-ac06-6da9373f97f3 - Code Review Stack Exchange

#Methods & Naming

You're building a collection so, implicitly, you're building a contract with your user that your class will act like a collection, that you'll stick to a protocol for collections objects.

If you wan't to grasp a little more on what I’m trying to say, you can read [this article][1].

So, even though I agree with [@Dair][2] that `size` would be “saffer” as `_size` (but hey, [we’re all responsible adults around here][3]), I strongly disagree with the `size` property. You should implement it as a `__len__` method and use it like:

 stuff = LinkedList()
 length = len(stuff)

Some other methods can have more common names too:

 * `add` \$->\$ `apppend`;
 * `add_at` \$->\$ `insert` or `__setitem__`:

 `__setitem__` will allow you to do `stuff[3] = 'some value'` and then `insert` is just a call to `self[index] = value`.

 You will need to change the signature to `def __setitem__(self, key, value)`.
 * `remove` \$->\$ `pop` (or maybe `__delitem__`); `remove` is prefered to remove by value instead of by index.
 * `get` \$->\$ `__getitem__`: it will allow you to do `the_value = stuff[3]`; you should also remove `get_node` as there is little to no interest in exposing your internals to the user.

#Methods that are missing

You could implement an `__iter__` method that will allow you to do:

 stuff = LinkedList()
 # populate stuff
 for elem in stuff: # will use __iter__, or fail back to __len__ + __getitem__ if not available
 do_something(elem)

It will also help you shorten `__getitem__`. You can use something along the lines of:

 def __iter__(self):
 node = self.first
 while node is not None: # No self.size involved, yay
 yield node.value
 node = node.after

You can also implement the `__reversed__` iterator using the same logic (and use it in `__getitem__` too.

Using a parameter to `__init__` that default to `None` would also be usefull to build a `LinkedList` out of any iterable:

 def __init__(self, other=None):
 self._size = 0
 self.first = None
 self.last = None

 if other is not None:
 for element in other:
 self.append(element)

It is then easy to create `stuff = LinkedList([1,2,8,12])`.

And finally, you should consider adding a `__str__` or `__repr__` method so you can `print(stuff)`. Use `__iter__` to simplify it.


 [1]: http://lucumr.pocoo.org/2011/7/9/python-and-pola/
 [2]: https://codereview.stackexchange.com/a/114820/84718
 [3]: http://stackoverflow.com/a/2064400/5069029