10
\$\begingroup\$

I am given a task to ask for a number and then check if it is present within a linked list or not.

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

    def getData(self):
        return self.data

    def getNext(self):
        return self.next

    def setData(self,newdata):
        self.data = newdata

    def setNext(self, newnext):
        self.next = newnext


class Linkedlist:
    def __init__(self):
        self.head = None

    def add_member(self,data):
        temp = Node(data)
        temp.setNext(self.head)
        self.head = temp

    def search_member(self,item):
        current = self.head
        while current != None:
            if current.getData() ==item:
                return True
            else:
                current =current.getNext()
        return False

llist = Linkedlist()
llist.add_member(23)
llist.add_member(98)
llist.add_member(415)
llist.add_member(123)
llist.add_member(981)
llist.add_member(454)
llist.add_member(213)
llist.add_member(198)
llist.add_member(455)
llist.add_member(253)
llist.add_member(978)
llist.add_member(45)
llist.add_member(203)
llist.add_member(918)
llist.add_member(45)
item = int(raw_input("Enter the number you want to search : "))
print llist.search_member(item)

How can I improve this code?

\$\endgroup\$

2 Answers 2

17
\$\begingroup\$
  1. First, you need to realize that Python is not Java. Python is a "consenting adult language." (Watch Raymond Hettinger's talk for more.)

    What that means is the bizarre fetish for mutators/accessors/getters and setters that permeates Java and C# doesn't exist in Python. You don't need to write:

    class Node:
        def getData(self):    
        def getNext(self):
        def setData(self,newdata):
        def setNext(self, newnext):
    

    Instead, you can just write this:

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

    And let people access node.data and node.next directly. (Python provides a mechanism, @property, for dealing with the case where you want to turn a member access into a function call. But it's not the first thing to do.)

  2. The next thing that struck me about your code was this:

    llist = Linkedlist()
    llist.add_member(23)
    llist.add_member(98)
    llist.add_member(415)
    llist.add_member(123)
    

    That's a lot of letters to get 4 numbers into a list. You can, and should, do better!

    "How can I do this better," you ask? Well, post it on CodeReview of course! But also, consider how the Python list class (and set, and dict, and tuple, and ...) is initialized. And how the Mutable Sequence Types are expected to work.

    Because your code is implementing a "mutable sequence type." So there's no reason that your code shouldn't work the same way. In fact, if you want other people to use your code, you should try to produce as few surprises as possible. Conforming to an existing interface is a good way to do that!

  3. Create an initializer that takes a sequence.

    class LinkedList:
        def __init__(self, seq=None):
            ...
            if seq is not None:
                self.extend(sequence)
    
  4. Implement as many of the mutable sequence operations as possible.

  5. Use the standard method names where possible: clear, extend, append, remove, etc.

  6. Implement special dundermethods (method names with "double-underscores" in them: double-under-methods, or "dundermethods") as needed to make standard Python idioms work:

    def __contains__(self, item):
        for i in self:
            ...
    
    def __iter__(self):
        node = self.head
    
        while node:
            yield node.value
            node = node.next
    
  7. Implement your test code using standard Python idioms, to prove it's working:

    llist = LinkedList([23, 98, 415, 123, 981, 454, 213, 198, ...])
    
    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\$
1
  • \$\begingroup\$ At point 3 you mean seq instead of sequeance, don't you? \$\endgroup\$ Commented Jan 15, 2018 at 14:10
8
\$\begingroup\$

This isn't a linked list

Its interface is that of a set -- the only options supported are add item, and check for membership.

Its implementation is that of a stack -- you can only add at one end, and searches start from that end.

Read up on PEP-8

PEP-8 is the standard for writing Python code. I only see two problem:

  • One space around the binary operators like = and ==. You do this most of the time, but try to be completely consistent.
  • Be sure to include docstrings on all public methods. PEP-257 talks about docstrings. These are short, triple-quoted strings right after the def line of each function that explain the purpose of the function, and what the parameters mean.

Why not make a better constructor for Node?

You always set both the item and the next, so pass those both in.

Take advantage of falsiness of None:

while current != None:

should become

while current:

Stick your test code in a main() function

So that it can be called when appropriate. You don't want the test cases to run automatically whenever the module is imported, so don't put the code at toplevel.

And call that function from a main() guard:

if __name__ == '__main__':
     main()

So that when you run the module the usual way, it will trigger the test cases.

\$\endgroup\$
1
  • \$\begingroup\$ Disagree with your first point; this data structure is definitely implemented as a linked list. I agree, though, that it doesn't offer all the methods that one might expect from a linked list interface, such as removal. \$\endgroup\$
    – flornquake
    Commented Jan 14, 2018 at 14:13

Your Answer

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

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