5
\$\begingroup\$

I made a simple program in Python

  • stores tasks in the file
  • allows the user to manage them.

I'm interested if my implementation is efficient and could I do something better. I'm not sure whether loading all data from file to memory and again saving it in the file is a good way to implement list saving tasks.

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import os


CHOICE_LIST_TASKS = 1
CHOICE_ADD_TASK = 2
CHOICE_REMOVE_TASK = 3
CHOICE_QUIT = 4


def read_int(prompt='> ', errmsg='Invalid number!'):
    num = None
    while num is None:
        try:
            num = int(input(prompt))
        except ValueError:
            print(errmsg)
    return num


def display_menu():
    print('What do you want to do?')
    print('[1] List all tasks')
    print('[2] Add a new task')
    print('[3] Delete task')
    print('[4] Quit')


choice = None
file = None
if not os.path.exists('tasks'):
    file = open('tasks', 'w+')
else:
    file = open('tasks', 'r+')
tasks = []

for line in file.readlines():
    tasks.append(line.strip())

while choice != CHOICE_QUIT:
    display_menu()
    choice = read_int()
    if choice == CHOICE_LIST_TASKS:
        if len(tasks) == 0:
            print('Task list is empty!')
        else:
            print('Tasks:')
            for x in tasks:
                print(x)
    elif choice == CHOICE_ADD_TASK:
        desc = input('Which task do you want to do? ')
        index = tasks.append(desc)
        print('Successfully added a new task!')
    elif choice == CHOICE_REMOVE_TASK:
        if len(tasks) == 0:
            print('Task list is empty!')
        else:
            for x in tasks:
                print(tasks.index(x), x)
            index = read_int('Which task do you want to delete? ')
            try:
                tasks.pop(index)
                print('Successfully deleted task!')
            except IndexError:
                print('Please enter proper task number')
    elif choice == CHOICE_QUIT:
        print('Good bye!')
    else:
        print('Invalid choice!')

file = open('tasks', 'w+')
for x in tasks:
    file.write(x + '\n')
\$\endgroup\$
  • 3
    \$\begingroup\$ you started off strong with functions, and then its all one big blob of code segment? \$\endgroup\$ – hjpotter92 Oct 7 at 21:09
  • \$\begingroup\$ consider accepting an answer \$\endgroup\$ – Aryan Parekh Oct 13 at 18:27
3
\$\begingroup\$

Let's start from the very top.

CHOICE_LIST_TASKS = 1
CHOICE_ADD_TASK = 2
CHOICE_REMOVE_TASK = 3
CHOICE_QUIT = 4

It's clear to me that you want to create variables and assign them certain integers so you don't have to use magic numbers anywhere, kudos to you for this. There is a much better way to implement this idea in python

Enumerations : Enum

Enumerations in Python

An enumeration is a set of symbolic names
(members) bound to unique, constant values

creating an Enum

class Choices(Enum):
    list_task = 1
    add_task = 2 
    remove_task = 3
    quit = 4

This already looks much clearer than random global variables. You can also iterate through the enumeration and display its values with names .

print(Choices.add_task.name)

add_task

print(Choices.add_task.value) 

1

Use more functions

file = None
if not os.path.exists('tasks'):
    file = open('tasks', 'w+')
else:
    file = open('tasks', 'r+')
tasks = []

for line in file.readlines():
    tasks.append(line.strip())

Why can't we move this into a function called read_task_file()?
This way it is extremely clear to me, you, you after a year reading old projects, and anybody else who will ever read this code that those few lines of code will read the task file. Without that, one must read the code thoroughly and understand its purpose.

def read_task_file():
    file = None
    if not os.path.exists('tasks'):
        file = open('tasks', 'w+')
    else:
        file = open('tasks', 'r+')
    tasks = []
    for line in file.readlines():
        tasks.append(line.strip())
    return tasks

now getting the tasks is

tasks = read_task_file()

This exact same applies to the next set of code, which will let the user enter their choice so we can perform it.

Factoring out the part where we read a choice from the user and perform into a function would look like

def read_task_choice():
    display_menu()
    choice = read_int()
    if choice not in Choices._value2member_map_:
        print("Invalid input! Please enter a correct choice\n")
        read_task_choice()

Note: Choices._value2member_map_ will be all the values in an enumeration.

This already looks much cleaner! We eliminate the need of having it in a loop where it will check all the cases, the function only returns choice when there is a valid input.

As I said, factor out the work into functions and all them when required. I have done the same for the 4 main jobs ( the 4 choices ).


List tasks

def list_tasks(tasks):
    if len(tasks) == 0:
        print("Task list is empty!")
        return None


    for i in range(len(tasks)):
        print(f"{i} {tasks[i]}")

Add new task

def add_new_task(tasks):
    return input("What task would you like to finish ?: ")
    tasks.append(task)

Remove task

def remove_task(tasks):
    if len(tasks) == 0:
        print("Task list is already empty!")
        return None

    list_tasks(tasks)
    task = int(input(("Which task would you like to remove: ?")))

    if task < 0 or task >= len(tasks):
        print("Invalid input! Please select an appropriate task")
        remove_task(tasks)
    tasks.pop(task)

And finally

Update task file

def update_task_file(tasks):
    file = open('tasks', 'w+')
    for x in tasks:
        file.write(x + '\n')

What is the point of having millions of functions when I can put everything like I have already done? Now when you have to play the game

tasks = read_task_file()

while True:
    choice = read_task_choice()
    if choice == Choices.list_task.value:
        list_tasks(tasks)

    elif choice == Choices.add_task.value:
        add_new_task(tasks)
        

    elif choice == Choices.remove_task.value:
        remove_task(tasks)

    else:
        break

    update_task_file(tasks)

This is the main advantage of having strongly typed functions.

Clearing the screen and waiting for input

Due to the absence of Graphics, we can see that after a few choices the terminal looks a little strangeterminal

We can improve this by simply clearing the screen after each choice, and then wait for the user to press a key

Clearing the terminal

There are a few ways. A popular way is to use os.system("cls") if you're on windows, but calling this is quite expensive and also makes your program more platform dependant.

You can do

print(chr(27) + "[2J")

And this will also work.

Waiting for input

Adding this line input("Press any key to continue...") will wait for the user's response before displaying all the choices again, making the experience better.

Final

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import os
from enum import Enum

class Choices(Enum):
    list_task = 1
    add_task = 2
    remove_task = 3
    quit = 4

def read_int(prompt='> ', errmsg='Invalid number!'):
    num = None
    while num is None:
        try:
            num = int(input(prompt))
        except ValueError:
            print(errmsg)
    return num

def display_menu():
    print('What do you want to do?')
    print('[1] List all tasks')
    print('[2] Add a new task')
    print('[3] Delete task')
    print('[4] Quit')

def read_task_file():
    file = None
    if not os.path.exists('tasks'):
        file = open('tasks', 'w+')
    else:
        file = open('tasks', 'r+')
    tasks = []
    for line in file.readlines():
        tasks.append(line.strip())
    return tasks

def update_task_file(tasks):
    file = open('tasks', 'w+')
    for x in tasks:
        file.write(x + '\n')

def read_task_choice():
    display_menu()
    choice = read_int()
    if choice not in Choices._value2member_map_:
        print("Invalid input! Please enter a correct choice\n")
        read_task_choice()
    return choice

def add_new_task(tasks):
    task = input("What task would you like to do: ")
    tasks.append(task)

def list_tasks(tasks):
    if len(tasks) == 0:
        print("Task list is empty!")
        return None


    for i in range(len(tasks)):
        print(f"{i} {tasks[i]}")

def remove_task(tasks):
    if len(tasks) == 0:
        print("Task list is already empty!")
        return None

    list_tasks(tasks)
    task = int(input(("Which task would you like to remove: ?")))

    if task < 0 or task >= len(tasks):
        print("Invalid input! Please select an appropriate task")
        remove_task(tasks)
    tasks.pop(task)

def clear_screen():
    print(chr(27) + "[2J")

def pause():
    input("Press any key to continue...")

tasks = read_task_file()

while True:
    clear_screen()
    choice = read_task_choice()
    if choice == Choices.list_task.value:
        list_tasks(tasks)

    elif choice == Choices.add_task.value:
        add_new_task(tasks)


    elif choice == Choices.remove_task.value:
        remove_task(tasks)

    else:
        break

    update_task_file(tasks)
    pause()

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