9
\$\begingroup\$

I made this to record the time I spend practicing Python. I'll eventually add in functionality to visualize the data. I'd appreciate any feedback that will help me improve.

# timer.py - A timer to track time spent practicing python

from datetime import timedelta
from datetime import datetime as Datetime
# used json b/c I wanted to practice it
import json
import os

# dict with all python practice options
options = ["Code Review",
           "Project",
           "Puzzles (Project Euler, Finxter, kaggle)",
           "Courses, Books, or Videos",
           "Read Docs"]


# Prompts user what type of practice they would like to do. User enters int.
def practice_choice():
    print("Pick a form of practice:\n")

    # print each option in options then ask user to select
    num = 1
    for option in options:
        print(str(num) + ". " + option)
        num += 1
    selection = input("\nPick a number: ")

    return options[int(selection) - 1]


def timer(selection):
    # store notes about practice session
    notes = input("Enter notes... ")

    # start timing session and get start time
    input("Hit Enter to begin practice...")
    start_dt = Datetime.now()

    # loop to allow for pausing. infinite loop that breaks when user clicks enter to stop session.
    var = 1
    total_so_far = 0
    while var == 1:
        pause_or_stop = input("Hit Enter to stop or space to pause...")

        # when user hits enters space char
        if pause_or_stop == ' ':

            # gets time session was paused. Calculates time practicing so far. Stores for later
            # Allows user to pause multiple times
            pause_dt = Datetime.now()
            total_so_far += int((start_dt - pause_dt) / timedelta(seconds=1))  # used sec for easier testing. 

            # resume on enter key and overwrites old start time
            input("Hit Enter to resume")
            start_dt = Datetime.now()

        # breaks if enter key
        elif pause_or_stop == '':
            break

        # catch bad input
        else:
            print("Key not recognized")

    # get stop time and find total time in minutes
    end_dt = Datetime.now()
    total_mins = int((end_dt - start_dt) / timedelta(seconds=1)) + total_so_far  # used sec for easier testing. 

    # add value to data[selection] dict. value is a list of datetime info, noes, total_minutes
    data[selection].append({'day': Datetime.now().strftime("%A"),  # %b %d %Y %H:%M),
                            'date': Datetime.now().strftime("%m-%d-%y"),
                            'time': Datetime.now().strftime("%H:%M"),
                            'notes': notes,
                            'minutes': total_mins})

    # convert dict to JSON object
    with open(target_file, 'w') as json_file:
        json.dump(data, json_file, indent=4)


# prompt user for type of practice
choice = practice_choice()

# practice_log file with all practice sessions are stored
target_file = "practice_log.json"

# initialize data dict which will hold practice logs
data = {}

# creates json file if it doesn't exist
if not os.path.exists(target_file):
    # top level values of json are created corresponding to practice type
    for each_item in options:
        data[each_item] = []
    with open(target_file, 'w') as json_file:
        json.dump(data, json_file, indent=4)

# open json file and load into target_files
with open(target_file, 'r') as json_file:
    data = json.load(json_file)

# starts timer
timer(choice)
\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

Ways to improve and redesign:

I'll start with listing inner issues (duplication, redundant or over-complicated conditional/algorithms etc). Then I'll provide you with a new Object-Oriented approach as a more manageable, flexible and reliable.

  • options = [...]. "Practice options" is better designed as a constant with immutable items (tuple):

    OPTIONS = ("Code Review",
            "Project",
            "Puzzles (Project Euler, Finxter, kaggle)",
            "Courses, Books, or Videos",
            "Read Docs")
    
  • practice_choice function:
    Dragging num variable though the for loop is redundant and easily replaced with enumerate feature (with custom start= parameter):

    print('\n'.join(f'{i}. {option}' for i, option in enumerate(self.OPTIONS, 1)))
    
  • timer function:
    var variable here:

    var = 1
    total_so_far = 0
    while var == 1:
        ...
    

    is redundant and it's not reassigned anywhere. Simply go with:

    while True:
        ...
    
  • total_so_far variable is better named as total_time

  • Datetime.now() is duplicated on constructing a dictionary of session info (notes, total minutes etc).
    Instead, use end_dt from the nearest statement end_dt = Datetime.now() as it's already contains the current datetime.

  • creating non-existent target file:
    filling data dict with:

    for each_item in options:
        data[each_item] = []
    

    is easily substituted with dict comprehension:

    data = {o: [] for o in self.OPTIONS}
    
  • saving data

    with open(target_file, 'w') as json_file:
        json.dump(data, json_file, indent=4)
    

    is extracted into a separate function/method with respective name


Now, the Object-Oriented PracticeTimer class with lot of benefits like:

  • class constants
  • encapsulated state/behavior
  • dealing with target file: initializing/saving
  • methods with specific responsibilities
  • easy to extend

Underlying Class diagram:

enter image description here


# timer.py - A timer to track time spent practicing python

from datetime import timedelta
from datetime import datetime as Datetime
import json
import os


class PracticeTimer:
    # practice options
    OPTIONS = ("Code Review",
               "Project",
               "Puzzles (Project Euler, Finxter, kaggle)",
               "Courses, Books, or Videos",
               "Read Docs")

    # practice_log file with all practice sessions are stored
    TARGET_FILE = "practice_log.json"

    def __init__(self):
        # creates json file if it doesn't exist
        if not os.path.exists(self.TARGET_FILE):
            # top level values of json are created corresponding to practice type
            data = {o: [] for o in self.OPTIONS}
            with open(self.TARGET_FILE, 'w') as json_file:
                json.dump(data, json_file, indent=4)

        # open json file and load session data
        with open(self.TARGET_FILE, 'r') as json_file:
            self._data = json.load(json_file)

        self._current_choice = None

    def practice_choice(self):
        """Prompts user to select practice item. User enters int."""
        print("Pick a form of practice:\n")

        # print options, then ask user to select
        print('\n'.join(f'{i}. {option}' for i, option in enumerate(self.OPTIONS, 1)))
        choice = input("\nPick a number: ")
        self._current_choice = self.OPTIONS[int(choice) - 1]
        return self._current_choice

    def _save_log(self):
        with open(self.TARGET_FILE, 'w') as json_file:
            json.dump(self._data, json_file, indent=4)

    def run(self):
        self.practice_choice()

        # store notes about practice session
        notes = input("Enter notes... ")

        # start timing session and get start time
        input("Hit Enter to begin practice...")
        start_dt = Datetime.now()

        # loop to allow for pausing. Infinite loop that breaks when user clicks enter to stop session.
        total_time = 0
        while True:
            pause_or_stop = input("Hit Enter to stop or space to pause...")

            # when user hits enters space char
            if pause_or_stop == ' ':

                # gets time session was paused. Calculates time practicing so far. Stores for later
                # Allows user to pause multiple times
                pause_dt = Datetime.now()
                total_time += int((start_dt - pause_dt) / timedelta(seconds=1))  # used sec for easier testing.

                # resume on enter key and overwrites old start time
                input("Hit Enter to resume")
                start_dt = Datetime.now()

            # breaks if enter key
            elif pause_or_stop == '':
                break
            # catch bad input
            else:
                print("Key not recognized")

        # get stop time and find total time in minutes
        end_dt = Datetime.now()
        total_mins = int((end_dt - start_dt) / timedelta(seconds=1)) + total_time  # used sec for easier testing.

        # add info to the current session (as a list of datetime info, notes, total_minutes)
        self._data[self._current_choice].append({'day': end_dt.strftime("%A"),  # %b %d %Y %H:%M),
                                                 'date': end_dt.strftime("%m-%d-%y"),
                                                 'time': end_dt.strftime("%H:%M"),
                                                 'notes': notes,
                                                 'minutes': total_mins})

        self._save_log()


if __name__ == '__main__':
    p_timer = PracticeTimer()
    p_timer.run()
\$\endgroup\$
5
  • 2
    \$\begingroup\$ Nice use of a class. I was contemplating whether I should add it, but decided against it in the end. Good to see that it is represented nevertheless! \$\endgroup\$
    – Graipher
    Commented Nov 7, 2019 at 10:13
  • \$\begingroup\$ @Graipher, Thanks, I appreciate that. \$\endgroup\$ Commented Nov 7, 2019 at 10:18
  • \$\begingroup\$ (# used sec for easier testing., if you put these in a row just above the code it refers to, you won't have to scroll right here on SO. :) ) \$\endgroup\$
    – BruceWayne
    Commented Nov 7, 2019 at 22:13
  • \$\begingroup\$ Thanks! I really appreciate this. A couple questions so far: - At the top, you use self.OPTIONS in the practice_choice function. I shouldn't be able to use self until I implemented the PracticeTimer class, correct? - In practice_choice, why do you use \n in the join? Doesn't that separate each character with a newline? \$\endgroup\$ Commented Nov 9, 2019 at 22:44
  • \$\begingroup\$ @engineer.lisa, welcome. 1) OPTIONS attribute will be accessed and shared across all PracticeTimer instances, though it resides in the class namespace. Python will take care of that due to name resolution. If you need to visually point out the exact binding - you may specify it as PracticeTimer.OPTIONS; 2) Doesn't that separate each character with a newline? - No, that one-liner with single print call is a replacement of your for loop with multiple print calls, it concatenates each "option" with a newline \n to get the needed output \$\endgroup\$ Commented Nov 10, 2019 at 8:02
7
\$\begingroup\$

Welcome to Code Review! This is a nice first question. Here are a few comments:

  • Documenting your code is nice. Comments are an OK way to do it, but one hard part of documentation is already obvious in your first comment:

    # dict with all python practice options
    options = ["Code Review", ...]
    

    Keeping code and documentation in line is hard. You say it is a dict, but it is actually a list.

    For some of your other comments, you should take a look at PEP257, the description of the docstring. This allows you to set a documentation string which can be interactively retrieved using help(...) and which is picked up by many automatic documentation tools. For your function practice_choice this could be as simple as moving the comment inside:

    def practice_choice():
        """Prompts user what type of practice they would like to do. User enters int."""
        ...
    
  • The standard library is your friend. It has many built-in functions and many more helpful modules. It never hurts trying to learn as many of them as possible.

    Instead of

    num = 1
    for option in options:
        print(str(num) + ". " + option)
        num += 1
    

    You can write:

    for num, option in enumerate(options, 1):
        print(f"{num}. {option}")
    

    This uses enumerate (using the optional second argument to start at one) and the new (Python 3.7+) f-string.

    Similarly, when the file is newly created, you have to create empty lists so that later the calls to data.append don't fail. Instead you could use collections.defaultdict, which is also in the standard library:

    from collections import defaultdict
    
    data = defaultdict(list)
    

    This will call the given constructor, here list, whenever a key does not yet exist, giving you an empty list.

  • If you are writing an interactive program, it pays off to define a generic enough function to handle user input. Common features you want is type adherence, allowed values, ranges, or even generic validator functions, and prompting the user until a valid input is given. This is how such a function could look like in your case:

    def ask_user(message, type_=str, valid=None):
        while True:
            user_input = input(message)
            try:
                user_input = type_(user_input)
            except ValueError:
                print(f"Wrong type, expected {type_}")
                continue
            if valid is not None:
                if callable(valid) and valid(user_input):
                    return user_input
                elif user_input in valid:
                    return user_input
                print("Invalid input.")
    

    This can be used in many places in your code:

    selection = ask_user("\nPick a number: ", int, range(1, len(options) + 1))
    
    msg = "\n".join(f"{num}. {option}" for num, option in enumerate(options, 1))
    choice = options[ask_user(msg, int, range(1, len(options) + 1)) - 1]
    
    ask_user("Hit Enter to begin practice...", valid="")
    
    pause_or_stop = ask_user("Hit Enter to stop or space to pause...", valid=" ")    # or `valid=["", " "]`, but `"" in " "` returns True
    
  • Try to separate your functions that do something from input and output. You have the core functionality of the function, a pausable timer, and then you have taking notes associated with it, saving it to a file, adding metadata. I would put these things in separate functions.

    Along with that, you should put your main code under a if __name__ == "__main__": guard, to allow importing from this file without executing the code.

    Here is one way to do it. This is a pausable timer as a generator that yields the beginning and end of every section

    def timer():
        start = datetime.now()
        print("Timer started")
        while True:
            user_input = ask_user("Hit Enter to stop or space to pause...", valid=" ")
            yield start, datetime.now()
            if user_input != " ":
                return
            if ask_user("Hit Enter to resume, anything else to abort", valid=" ") == "":
                start = datetime.now()
            else:
                return
    

    Which can be used like this:

    for t0, t1 in timer():
        total_mins = int((t1 - t0).total_seconds() // 60)
        ...
    

    The whole main function could then be:

    def main():
        options = ["Code Review",
                   "Project",
                   "Puzzles (Project Euler, Finxter, kaggle)",
                   "Courses, Books, or Videos",
                   "Read Docs"]
        choice = practice_choice(options)
        notes = input("Enter notes... ")
    
        file_name = "practice_log.json"
        if os.path.isfile(file_name):
            with open(file_name) as f:
                data = defaultdict(list, json.loads(f))
        else:
            data = defaultdict(list)
    
        for t0, t1 in timer():
            total_mins = int((t1 - t0).total_seconds() // 60)
            print(f"Worked on {choice} for {total_mins} minutes")
            data[choice].append({'day': t1.strftime("%A"),
                                 'date': t1.strftime("%m-%d-%y"),
                                 'time': t1.strftime("%H:%M"),
                                 'notes': notes,
                                 'minutes': total_mins})
            with open(file_name, 'w') as json_file:
                json.dump(dict(data), json_file, indent=4)
    
    if __name__ == "__main__":
        while True:
            main()
    

    Note that practice_choice now takes the options as an argument. I also made the main program repeat, in case you want to start the next timer immediately.

    I used os.path.isfile instead of os.path.exists, because the latter is also true for directories. I don't know why you would want to create a directory called "practice_log.json", but it can happen accidentally and then your code would crash.

  • Python has an official style-guide, PEP8. Your code follows it almost everywhere, but the one place it doesn't is where the standard library also doesn't.

    You imported it as Datetime, PEP8 would recommend DateTime, but it is just called datetime. I would either use the name it is given (which happened before the naming convention existed AFAIK), or use the fully PEP8 compliant name.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for all this feedback. Still going though it but wanted to say thanks. Also, I'll read the docstring PEP as you recommend. \$\endgroup\$ Commented Nov 9, 2019 at 22:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.