# Todo-list practice code accepts title and list

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
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)

• Welcome to Code Review. Perhaps you could also add some sample calls to show how you intend to use this code? – 200_success Jul 30 '16 at 3:48
• 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") – FATZOMBI Jul 30 '16 at 4:04

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.

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)

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))

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.