5
\$\begingroup\$

This program allows users to manage questions and quiz users from years of historical events. I'm looking for suggestions on how can I better organize my code, maybe apply OOP for functions and avoid using global variable data_filename.

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

from enum import Enum
import os
import pickle
import random


data_filename = 'program.obj'


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


def display_menu():
    print('What do you want to do?')
    print('[1] List all historical events')
    print('[2] Add event')
    print('[3] Remove event')
    print('[4] Quiz')
    print('[5] Statistics')
    print('[6] Clear statistics') # TODO: move to options submenu
    print('[7] Exit')


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


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


def yes_or_no(prompt='Proceed? [y|n]\n> ', errmsg='Valid answers are y and n.'):
    answer = input(prompt).strip().lower()
    while answer != 'y' and answer != 'n':
        print(errmsg)
        answer = input(prompt).strip().lower()
    return answer


def read_data_file():
    file = open(data_filename, 'a+b')
    file.seek(0)
    data = {'events': [], 'statistics': {'total_successes': 0, 'total_failures': 0}}
    if os.path.getsize(data_filename) > 0:
        data = pickle.load(file)
    file.close()
    return data


def list_events(data):
    events = data['events']
    if len(events) == 0:
        print('Event list is empty!')
    else:
        print('Historical events:')
        for event in events:
            print(event)


def add_event(data):
    events = data['events']
    year = read_int(prompt='Enter year: ')
    description = input('Enter description: ')
    events.append({'year': year, 'description': description})
    print('Successfully added a new historical event!')


def remove_event(data):
    events = data['events']
    if len(events) == 0:
        print('Event list is empty!')
    else:
        for event in events:
            print(events.index(event), event)
        index = read_int('Which event do you want to delete? ')
        try:
            events.pop(index)
            print('Successfully deleted event!')
        except IndexError:
            print('Number out of range!')


def quiz(data):
    events = data['events']
    stats = data['statistics']
    if len(events) == 0:
        print('Event list is empty!')
    else:
        num = read_int(prompt='How many questions should I ask? ')
        if 0 < num <= len(events):
            for event in random.sample(events, num):
                print(event['description'])
                year = read_int('In which year was following event occurred? ')
                if year == event['year']:
                    stats['total_successes'] += 1
                    print('Good answer!')
                else:
                    stats['total_failures'] += 1
                    print('Bad answer!')
        elif num < 0:
            print('Number of questions can\'t be negative!')
        else:
            print('Too much questions!')


def display_stats(data):
    stats = data['statistics']
    tries = stats['total_successes'] + stats['total_failures']
    if tries == 0:
        total_successes = 0
        total_failures = 0
    else:
        total_successes = stats['total_successes']/tries * 100
        total_failures = stats['total_failures']/tries * 100
    print('Statistics')
    print('Total: {0:10.2f}% successes, {1:10.2f}% failures'.format(total_successes, total_failures))


def clear_stats(data):
    answer = yes_or_no('Are you sure you want to clear statistics? [y|n]\n> ')
    if answer == 'y':
        data['statistics'] = {'total_successes': 0, 'total_failures': 0}
        print('Successfully cleared statistics!')
    else:
        print('Statistics left unchanged.')


# TODO: settings submenu
# def settings(data):
#     print('-' * 10)
#     print('Program settings')
#     print('-' * 10)
#     print('[1] Clear statistics')
#     print('[2] Back')
#     user_choice = read_int()
#     while user_choice != 2:
#         if user_choice == 1:
#             clear_stats(data)


def update_data_file(data):
    file = open(data_filename, 'wb')
    pickle.dump(data, file)
    file.close()


class Choices(Enum):
    list_events = 1
    add_event = 2
    remove_event = 3
    quiz = 4
    statistics = 5
    clear_stats = 6
    exit = 7


program_data = read_data_file()
choice = None
while choice != Choices.exit.value:
    clear_screen()
    display_menu()
    choice = read_int()
    if choice == Choices.list_events.value:
        list_events(program_data)
    elif choice == Choices.add_event.value:
        add_event(program_data)
    elif choice == Choices.remove_event.value:
        remove_event(program_data)
    elif choice == Choices.quiz.value:
        quiz(program_data)
    elif choice == Choices.statistics.value:
        display_stats(program_data)
    elif choice == Choices.clear_stats.value:
        clear_stats(program_data)
    elif choice == Choices.exit.value:
        print('Good bye!')
    else:
        print('Invalid choice!')
    update_data_file(program_data)
    pause()
\$\endgroup\$
  • 1
    \$\begingroup\$ awesome, i'd say the code is much more readable than before \$\endgroup\$ – Aryan Parekh Oct 17 at 9:57
  • \$\begingroup\$ As always with python, I suggest to apply black : pypi.org/project/black \$\endgroup\$ – Martin Thoma Oct 17 at 16:47
  • \$\begingroup\$ Use a "with" context handler for opening / closing files \$\endgroup\$ – Martin Thoma Oct 17 at 16:48
3
\$\begingroup\$

Constants

data_filename should be capitalized, since it's a global constant.

Early-return

There's no need to use number as the condition on your while; instead:

number = None
while number is None:
    try:
        number = int(input(prompt))
    except ValueError:
        print(errmsg)
return number

can be

while True:
    try:
        return int(input(prompt))
    except ValueError:
        print(errmsg)

Menu management

display_menu could use a sequence of tuples, better yet a sequence of named tuples or @dataclasses, each having a title string attribute and a callable attribute. Then your display_menu could be

print('What do you want to do?')
print('\n'.join(f'[{i}] {item.title}' for i, item in enumerate(menu, 1)))

I see that you also have a Choices enum. That's not bad, and you could potentially use both an enum and the suggestion above, so long as you have a dictionary of enum-choices-to-function-references.

Membership tests

answer != 'y' and answer != 'n'

can be

answer not in {'y', 'n'}

Loop structure

Avoid having to call input twice; this:

answer = input(prompt).strip().lower()
while answer != 'y' and answer != 'n':
    print(errmsg)
    answer = input(prompt).strip().lower()
return answer

can be

while True:
    answer = input(prompt).strip().lower()
    if answer in {'y', 'n'}:
        return answer
    print(errmsg)

File operations

file = open(data_filename, 'a+b')
file.seek(0)

has a few issues:

  • Newly-opened file handles do not need an initial seek-to-beginning; that's redundant if you open in read-binary mode
  • You should be using with and avoiding an explicit close()
  • You should only bother to initialize your default data when needed
  • You want to check whether the file exists, not its size

So:

if os.path.exists(data_filename):
    with open(data_filename, 'rb') as file:
        return pickle.load(file)
return {'events': [], 'statistics': {'total_successes': 0, 'total_failures': 0}}

Redundant predicates

if 0 < num <= len(events):
    ...
elif num < 0:
    print('Number of questions can\'t be negative!')
else:
    print('Too much questions!')

needs to be re-thought. First, too much questions should be too many questions. Also, what if the user enters 0? Surely that's not "too many questions", but that's what will be printed. Suggested:

if num < 1:
    print('Not enough questions.')
elif num > len(events):
    print('Too many questions.')
else:
    ...

Stronger-typed, stronger-structured data

stats['total_successes'] + stats['total_failures']

is an example of what I've seen called "data pasta". Dictionaries are being abused, here, when something like a @dataclass containing type hints is more appropriate.

String interpolation

'Total: {0:10.2f}% successes, {1:10.2f}% failures'.format(total_successes, total_failures)

is more easily expressed as

(
    f'Total: {total_successes:10.2f}% successes, '
    f'{total_failures:10.2f}% failures'
)

The line split is optional but better for legibility.

String escapes

'Number of questions can\'t be negative!'

is more easily written as

"Number of questions can't be negative!"
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I have an issue with your answer. If a file exists, without file.seek(0) I'm getting EOFError: Ran out of input. Otherwise, this program instead of creating a file and returning empty settings object throws exception FileNotFoundError. Code: dpaste.com/7ULK8KJYD \$\endgroup\$ – user231566 Oct 17 at 18:10
  • \$\begingroup\$ That's deeply strange. Is it a normal file, or a FIFO (etc)? \$\endgroup\$ – Reinderien Oct 17 at 18:11
  • \$\begingroup\$ This is a normal file. Maybe I'll send the complete project code? \$\endgroup\$ – user231566 Oct 17 at 18:12
  • \$\begingroup\$ Oh, I see what's happening. You open in append mode, but then seek? Why? \$\endgroup\$ – Reinderien Oct 17 at 18:13
  • 1
    \$\begingroup\$ Problem solved. Changed if statement condition to os.path.exists(DATA_FILENAME) and os.path.getsize(DATA_FILENAME) > 0. \$\endgroup\$ – user231566 Oct 17 at 18:22

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.