patternpythonMinor

Todo-list practice code accepts title and list

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

acceptspracticeandcodelisttodotitle

Problem

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)

Solution

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 : `

).

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:

remove corresponds to __delitem__

update corresponds to __setitem__

__len__ is fine.



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

 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.

Code Snippets

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 search(self, query):
    result = []
    for k, i in enumerate(self.items):
        if query in i:
            result.append(k)
    return self.print_list(result)

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

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

Context

StackExchange Code Review Q#136376, answer score: 7

Revisions (0)

No revisions yet.