4
\$\begingroup\$

A follow up for this question: Implementing Linked lists in Python from scratch

Node:

class Node():
    def __init__(self, key):
        self.key = key
        self.next = None

Linked list:

from node import Node


class LinkedList:
    link_connection = ' -> '

    ''' init, getters, setters '''
    def __init__(self, key):
        self.head = Node(key)
        self.tail = self.head
        self.length = 1

    @property
    def head(self):
        return self.__head

    @head.setter
    def head(self, node):
        if node is not None:
            self.__head = node

    @property
    def tail(self):
        return self.__tail

    @tail.setter
    def tail(self, node):
        if node is not None:
            try:
                if node.next is None:
                    self.__tail.next = node
                    self.__tail = self.__tail.next
            except AttributeError:
                self.__tail = node

    @property
    def length(self):
        return self.__length

    @length.setter
    def length(self, value):
        self.__length = value

    ''' generator for nodes '''
    ''' iterating, search etc. '''
    def node_generator(self, target=None):
        node = self.head
        yield node
        while node.next is not None and node.next.key != target:
            node = node.next
            yield node

    def __iter__(self):
        return map(lambda node: node.key, self.node_generator())

    def __str__(self):
        return self.link_connection.join(str(key) for key in self)

    def go_forward(self, steps):
        for node in self.node_generator():
            if steps == 0:
                return node
            steps -= 1

    def delete(self, target):
        for node in self.node_generator(target):
            pass
        node.next = node.next.next
        self.length -= 1

    def __len__(self):
        return self.__length

    def append(self, key):
        node = Node(key)
        self.tail = node
        self.length += 1

    def find(self, value):
        for node in self:
            if node == value:
                return node

Is this new implementation better?

\$\endgroup\$
4
\$\begingroup\$

Specification and Unit Tests

  • You are lacking a clear specification for your methods. We have to guess what these methods do exactly, specially for edge cases.
  • You should provide unit tests when providing an API class. You would have found some bugs if you did.

Review

 @head.setter
    def head(self, node):
        if node is not None:
            self.__head = node

There is no link from the new head to the previous one. Also, what would you do if node.next is already filled?

 @head.setter
    def head(self, node):
        if node is not None:
            node.next = self.__head
            self.__head = node

@tail.setter
    def tail(self, node):
        if node is not None:
            try:
                if node.next is None:
                    self.__tail.next = node
                    self.__tail = self.__tail.next
            except AttributeError:
                self.__tail = node

Why would you expect an AttributeError here? You are only setting next. And why would you ignore a tail that has a next node? Why not include the tail chain?


''' generator for nodes '''
''' iterating, search etc. '''
def node_generator(self, target=None):
    node = self.head
    yield node
    while node.next is not None and node.next.key != target:
        node = node.next
        yield node

The spec is unclear here. You always include the head, and then continue as long as a next node's key does not match the specified target. Why not check head for the same condition?


def delete(self, target):
    for node in self.node_generator(target):
        pass
    node.next = node.next.next
    self.length -= 1

What if the node you want to delete is the tail? This edge case is not foreseen.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ about the generator, it's because I want to return the node before the target, any suggestion to do that better? \$\endgroup\$ – Matan Cohen Jul 28 '19 at 14:42
  • 1
    \$\begingroup\$ If the head is the target, should you yield only the head, or no results? \$\endgroup\$ – dfhwze Jul 28 '19 at 14:44
  • 1
    \$\begingroup\$ OH! I see. Actually, I don't know what I want to return lol \$\endgroup\$ – Matan Cohen Jul 28 '19 at 14:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.