0
\$\begingroup\$

Below is my attempt at a doubly-linked list in Python (3):

class LinkedListNode:
    def __init__(self, value):
        self.value = value
        self.left  = None
        self.right = None


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

    def get_first(self):
        if self.head:
            return self.head.value
        raise Exception('Reading from an empty list.')

    def get_last(self):
        if self.tail:
            return self.tail.value
        raise Exception('Reading from an empty list.')

    def add_first(self, value):
        node = LinkedListNode(value)
        if self.head:
            node.right = self.head
            self.head.left = node
            self.size += 1
        else:
            self.head = self.tail = LinkedListNode(value)
            self.size = 1

    def add_last(self, value):
        node = LinkedListNode(value)
        if self.tail:
            node.left = self.tail
            self.tail.right = node
            self.size += 1
        else:
            self.head = self.tail = node
            self.size = 1

    def remove_first(self):
        if self.head:
            ret = self.head.value
            self.head = self.head.right
            self.head.left = None
            self.size -= 1
            return ret
        raise Exception('Removing from an empty list.')

    def remove_last(self):
        if self.tail:
            ret = self.tail.value
            self.tail = self.tail.left
            self.tail.right = None
            self.size -= 1
            return ret
        raise Exception('Removing from an empty list.')

    def __repr__(self):
        str = '['
        if self.size > 0:
            str += self.head.value
            node = self.head.right
            while node:
                str += ', ' + node.value
                node = node.right
        return str + ']'


def main():
    list = LinkedList()

    while True:
        cmd_line = input()
        if cmd_line == 'quit':
            break
        command_tokens = cmd_line.split()
        command = command_tokens[0]
        try:
            if command == "get_first":
                print(list.get_first())
            elif command == "get_last":
                print(list.get_last())
            elif command == "add_first":
                list.add_first(command_tokens[1])
            elif command == "add_last":
                list.add_last(command_tokens[1])
            elif command == "remove_first":
                list.remove_first()
            elif command == "remove_last":
                list.remove_last()
            elif command == "print":
                print(list)
        except Exception:
            print("Unknown command")


main()

How would I rewrite it in a professional manner?

\$\endgroup\$
2
  • \$\begingroup\$ Are you sure this is a version you want to review? It has gaping bugs - once the list has a node, neither self.head nor self.tail is ever updated anymore. \$\endgroup\$
    – vnp
    Jan 14, 2019 at 21:25
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Jan 15, 2019 at 14:33

1 Answer 1

2
\$\begingroup\$

A better main

Instead of just calling main() at the bottom, wrap it in if __name__ == "__main__":. In otherwords:

if __name__ == "__main__":
    main()

This allows me to re-use your module better, see the answer I linked for more details.

if chains

This portion of code:

        if command == "get_first":
            print(list.get_first())
        elif command == "get_last":
            print(list.get_last())
        elif command == "add_first":
            list.add_first(command_tokens[1])
        elif command == "add_last":
            list.add_last(command_tokens[1])
        elif command == "remove_first":
            list.remove_first()
        elif command == "remove_last":
            list.remove_last()
        elif command == "print":
            print(list)

Is a code smell to me. I would refactor this with a dictionary:

commands = {
    "get_first": lambda: print(list.get_first()),
    "get_last": lambda: prrint(list.get_last()),
    ...
}

This can then be called with:

commands[command]()

Library Shadowing

list = LinkedList()

list is already part of the standard library. I would not over shadow the name. I would change the name to something like linked for instance.

namedtuple?

It is questionable whether you want to do this or not, but the class LinkedListNode is just an __init__ method. In this case, I would usually advocate using a namedtuple instead and using the defaults parameter added in Python 3.7 if applicable. Although, then again, defaults is only in 3.7... So, maybe not applicable. Still useful to know about.

Implement __str__ how you implemented __repr__

__str__ is for human-readable output. __repr__ is for more internal information. You have implemented __repr__ to display a human readable representation of your list. Switch to __str__ instead.

\$\endgroup\$

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.