6
\$\begingroup\$

I am new to OOP principles and best practices. People often say I write "unpythonic code," so give me your perspective on how to do things better.

class TodoList(object):

    def __init__(self, title=None):
        self.title = title
        self.items = list()

    # depend on _list, accepts _list index numbers
    def __str__(self, tdl=None):

        if tdl is None:
            tdl=range(len(self.items))

        for i in tdl:
            item = "{0}. {1}".format(i, self.items[i])
            print(item)
        return str()

    def __len__(self):
        return len(self.items)

    # list bindings
    def add(self, item):
        self.items.append(item)

    def update(self, key, update):
        self.items[key] = update

    def remove(self, key):
        del self.items[key]

    # search through i, find query, append key
    def search(self, query):
        result = []
        for k, i in enumerate(self.items):
            if query in i:
                result.append(k)
        return self.__str__(result)
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Perhaps you could also add some sample calls to show how you intend to use this code? \$\endgroup\$ – 200_success Jul 30 '16 at 3:48
  • \$\begingroup\$ i really didn't thought about practicality of this code, as it was an excercise problem from an oop book, however: x = TodoList("Shopping list") x.add("buy potato") x.search("potato") \$\endgroup\$ – FATZOMBI Jul 30 '16 at 4:04
7
\$\begingroup\$

Your code looks nice and is properly formatted. Still, a few points could be improved.

Magic methods

As mentionned in ChrisWue's nice answer, the __str__ magic method is supposed to return a string and is not expected to have any side-effects (such as performing some printing). Also, I would not consider it a good idea to change the signature of a usual magic method. Finally, the string returned by the function is empty which does not bring much information to anyone (the string returned by default is somehow more interesting : <__main__.TodoList object at 0x6085be4ce470>).

The best way to improve this is to split your function into 2 functions: one __str__ with a correct behavior and another one. For instance:

def print_list(self, tdl=None):
    if tdl is None:
        tdl=range(len(self.items))

    for i in tdl:
        item = "{0}. {1}".format(i, self.items[i])
        print(item)

def __str__(self, tdl=None):
    return str(self.title)

This also corresponds to a better Separation of Concerns and to the Single Responsibility Principle : each entity should do/be responsible of a single thing.

More about magic methods

Among the methods you've defined, a few of them could be renamed to emulate a container type:

search

At the moment, search is defined like this (print_list being the function defined above):

def search(self, query):
    result = []
    for k, i in enumerate(self.items):
        if query in i:
            result.append(k)
    return self.print_list(result)

A few improvements could be done.

First, I'd use i as the variable name for the index and not for the item.

Then, the my_list = []; for ...: if ...: my_list.append(...) pattern is usually written using list comprehension which are more concise and clearer once you get used to it. They are also more efficient and considered more pythonic.

def search(self, query):
    result = [i for i, item in enumerate(self.items) if query in item]
    return self.print_list(result)

Finally, I'd consider it poor API to have a search method that doesn't return anything (while performing some printing). I'd like it more if you were to return the results and to have another method to pretty print them if needed.

The final code looks like (I am still not a big fan of print_list):

class TodoList(object):

    def __init__(self, title=None):
        self.title = title
        self.items = list()

    def print_list(self, tdl=None):
        if tdl is None:
            tdl = range(len(self.items))

        for i in tdl:
            item = "{0}. {1}".format(i, self.items[i])
            print(item)

    def __str__(self, tdl=None):
        return str(self.title)

    def __len__(self):
        return len(self.items)

    def add(self, item):
        self.items.append(item)

    def __setitem__(self, key, update):
        self.items[key] = update

    def __delitem__(self, key):
        del self.items[key]

    def search(self, query):
        return [i for i, item in enumerate(self.items) if query in item]

if __name__ == "__main__":
    x = TodoList("Shopping list")
    print(str(x))
    x.add("buy salad")
    x[0] = "buy potato"
    x.add("buy tomato")
    del x[1]
    x.print_list(x.search("potato"))

Going further

If you want to improve your code, you could consider adding documentation with docstrings. The docstring conventions for Python code can be found in a document called PEP 257.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I'm not doing a lot of python so I'm probably missing a few things. However two things I noticed:

  1. The search method returns a string representation of the found entries which is generally not very useful to a program using your class. You should simply return the result, or if you want to pass back the indices of the found entries as well then return a dictionary with the indices being the keys and the todo items being the values.

  2. Pretty sure __str__ is simply meant to return the human readable string but you are also printing things out which is probably unexpected to the caller.

| improve this answer | |
\$\endgroup\$

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.