1
\$\begingroup\$

This is a follow up question from here. I have revised my code as per the suggestions of peer reviewers. I still feel there is lot of scope for improvement in this code.

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

class LinkedList(object):

    def __init__(self):
        self.head = None
        self.size =0


    def extend(self, seq = None):
        """extends list with the given sequence"""

        for i in range(0, len(seq)):
            node = Node(seq[i])
            self.size +=1
            node.next = self.head
            self.head = node

    def append(self, item):
        """append item to the end of list"""
        node = Node(item)
        node.next = self.head
        self.head = node
        self.size += 1

    def printdata(self):
        """print elements of linked list"""
        node = self.head 
        while node:
            print node.data
            node = node.next

    def __iter__(self):
        node = self.head
        while node:
            yield node.data
            node = node.next

    def __contains__(self, item):
        """checks whether given item in list"""
        node = self.head 
        while node:
            if node.data == item:
                return True
            node = node.next


    def len(self):
        """returns the length of list"""
        return self.size


    def remove(self, item):
        """removes item from list"""
        node = self.head
        current = self.head.next
        if node.data == item:
            self.size -= 1
            self.head = current
            current = current.next
        while current:
            if current.data == item:
                current = current.next
                node.next = current
                self.size -= 1
            node = current
            current = current.next




    def __str__(self):
        return  str(self.data) + str(self.size)

test cases :

if __name__ == "__main__":

    llist = LinkedList()

    llist.extend([98,52,45,19,37,22,1,66,943,415,21,785,12,698,26,36,18,
    97,0,63,25,85,24])

    print "Length of linked list is ", llist.len()

    llist.append(222)

    print "Length of linked list is ", llist.len()

    llist.remove(22)

    print "Elements of linked list  \n", llist.printdata()
    print "Length of linked list is ", llist.len()

   ## Search for an element in list   
    while True:
        item = int(raw_input("Enter a number to search for: "))
        if item in llist:
            print "It's in there!"
        else:
            print "Sorry, don't have that one"
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  • The construct

    for i in range(0, len(seq)):
        node = Node(seq[i])
    

    is usually frowned upon. Consider a Pythonic

    for item in seq:
        node = Node(item)
    
  • I see no reason to default seq to None.

  • append does not append. It prepends.

  • remove has an unpleasant code duplication. To avoid special casing the head, consider using a dummy node:

    def remove(self, item):
        dummy = Node(None, self.head)
        prev = dummy
        while prev.next:
            if prev.next.data == item:
                prev.next = prev.next.next
                size -= 1
            prev = prev.next
        self.head = dummy.next
    
  • printdata may (or shall?) use the iterator:

    def printdata(self):
        for node in self:
            print node.data
    
\$\endgroup\$
1
\$\begingroup\$

I suggest the following:

  • Remove the default argument from extend. There is no upside to allowing client code to call a.extend() with no argument, since it's an error anyway.
  • Use the existing append function to simplify extend as follows:

    def extend(self, x): for item in x: self.append(item)

  • Use the iterator you have written wherever possible. Don't forget about the built-in function any. For example,

    def __contains__(self, a): return any(a == item for item in self)

  • In the iterator it would be better to change the while statement slightly (since None is the specific value that terminates the iteration):

    def __iter__(self): node = self.head while node is not None: yield node.data node = node.next

  • Since you never call the Node constructor with more than one argument, I would remove the second argument. I would also rename the class _Node so that client code is discouraged from messing with it (it exists solely to be a helper class for the linked list).

    class _Node(object): def __init__(self, data): self.data = data self.next = None

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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