4
\$\begingroup\$

This code is my first implementation of this style of application in the console. In essence a stopwatch with some added functionality. I was interested in getting some feedback before I refactor and begin adding more features. I'm fairly new to programming and I specifically have concerns about my design pattern.

Known issues:

  1. When the program saves a "session" (a json file) a directory is hard coded into the program. If this directory does not already exist an error occurs. I plan to fix this in the future.

  2. Size. The entire application is in one file, which is already causing me readability problems. All of my function definitions should be in a separate file at the very least.

  3. When I grab user input to change menus, I do not always check if it's valid input and notify the user when it isn't. Example: line 138 I do not have a case if the input is not a valid command. As opposed to something like line 110.

Concerns:

  1. Using while loops to change between "menus" or "states" has so far worked well, but I'm uncertain if my implementation is a good idea.

  2. Variable shadowing. For a single example, I use the variable command 5 times in this program, but the value of command is defined by user input and hence almost always different. There are numerous other examples in the code of similar or identical variable names. However, these usages are in separate while loops, so should I be concerned? If I do need to change my variable names, how do I do it and not be verbose/unclear?

  3. General code quality. Since I haven't been programming long I do not expect to have written stellar code. What are things I could do to improve the overall quality of this program?

Code:

import keyboard
import time
import json 
import os
from fuzzywuzzy import process 

def createSessionStructure():
    '''Creates a data structure for a session and adds a timestamp'''
    timestamp = time.ctime(time.time())
    return {"session": { 
            "timestamp": timestamp, 
            "times": [] 
            } 
        }

def cubeTime():
    '''Returns (and prints) floating point number under normal circumstance. Returns None if timer operation is aborted.'''
    print("Press space to begin timing, backspace to abort.")
    trigger = keyboard.read_event()
    if trigger.name == "backspace":
        print("Cancelled")
        return None
    else:
        start = time.perf_counter()
        keyboard.wait("space")
        end = time.perf_counter()
        keyboard.press_and_release("backspace") # These lines fix a bug where space characters would end up littering the menu after the timer closed
        keyboard.press_and_release("backspace") # By pressing backspace it clears any characters in the terminal to ensure a better user experience
        print(round((end - start), 4))
        return round((end - start), 4)

def storeSession(timeList):
    '''Writes list of times into a nested python dictionary structure'''
    session = createSessionStructure()
    for time in timeList:
        session["session"]["times"].append(time)
    return session

def writeSession(session, directory):
    '''Writes session data at filepath: directory. writeSession(exampleSession, "C:/foo/bar/") will create a json file inside /bar/ with the data from exampleSession'''
    command = input("Do you want to name this file (yes/no)? ")
    if command.lower() == "yes":
        customName = input("Name: ")
        outputFile = open(f"{directory}{customName}.json", "w+")
        json.dump(session, outputFile, indent=4)
        outputFile.close() 
    elif command.lower() == "no":
        timeStamp = session["session"]["timestamp"].replace(" ", "-").replace(":",".")
        outputFile = open(f"{directory}{timeStamp}.json", "w+")
        json.dump(session, outputFile, indent=4) #outputFile.write(jsonString) <- old method replaced with json.dump
        outputFile.close() 

def appendSession(timeList, filepath):
    '''Grabs old session data as a python object, appends new data to it, and overwrites the file with this new data'''
    with open(filepath, "r+") as JSONfile:
        sessionData = json.load(JSONfile)
        for time in timeList:
            sessionData["session"]["times"].append(time)
        JSONfile.seek(0)
        json.dump(sessionData, JSONfile, indent=4)

def fuzzyMatch(string, options):
    '''returns the best match from a list of options'''
    return process.extractOne(string, options)[0]

destination = "C:/Coding/cube-timer/times/"

PROGRAM_LOOP = True
mainMenu = True
dataMenu = False
cubeTimer = False
print("Welcome to Cube Timer!")
print("======================")
while PROGRAM_LOOP == True:
    while mainMenu == True:
        print("Main Menu")
        print("Commands: times, timer, or quit")
        command = input("Where would you like to go: ")
        if command.lower() == "times":
            mainMenu = False
            cubeTimer = False 
            dataMenu = True
        elif command.lower() == "timer":
            mainMenu = False
            cubeTimer = True 
            dataMenu = False
        elif command.lower() == "quit":
            PROGRAM_LOOP = False
            mainMenu = False
            dataMenu = False
            cubeTimer = False
        else:
            print("I don't understand that.")

    while dataMenu == True:
        globaltimes =  False
        sessiontimes = False
        command = input("Would you like to view global (gl), session (ses) times, or go back (back)? ")

        if command.lower() == "gl":
            globaltimes = True
            sessiontimes = False
        elif command.lower() == "ses":
            sessiontimes = True
            globaltimes = False
        elif command.lower() == "back":
            dataMenu = False
            cubeTimer = False
            mainMenu = True
        else:
            print("I don't understand that.")

        while sessiontimes == True:
            viewSession = False
            print("Which session would you like to view?\n")
            print(("-" * 20) + "\n")
            sessionFileNames = os.listdir("times/")
            for session in sessionFileNames:
                print(session)
            print(("-" * 20) + "\n")
            sessionFileChoice = input("filename: ")
            fuzzyFileName = fuzzyMatch(sessionFileChoice, sessionFileNames)
            sessionFilePath = 'times/' + fuzzyFileName
            with open(sessionFilePath, "r+") as JSONfile:
                viewSession = True
                sessionData = json.load(JSONfile)
                timestamp = sessionData["session"]["timestamp"]
                print(f"Session {fuzzyFileName} created at {timestamp}:\n")
                for time in sessionData["session"]["times"]:
                    print(time)
                print("\n")
                while viewSession == True:
                    command = input("Display average (av), or quit (quit): ")
                    if command.lower() == "quit":
                        viewSession = False
                        sessiontimes = False
                    elif command.lower() == "av":
                        print(sum(sessionData["session"]["times"]) / len(sessionData["session"]["times"]))

        while globaltimes == True:
            print("This area has not been implimented yet, returning to data menu.")
            globaltimes = False
            sessiontimes = False


    while cubeTimer == True:
        session = False
        command = input("Start a new session (new) or continue (cont) an old session? ")
        if command.lower() == "new":
            session = True
            updateSession = False
            sessionTimes = []
        elif command.lower() == "cont":
            print("Which session would you like to continue? ")
            stringFileNames = os.listdir("times/")
            for JSONfile in stringFileNames:
                print(JSONfile)
            fileChoice = input("filename: ")
            fuzzyPath = 'times/' + fuzzyMatch(fileChoice, stringFileNames)
            session = True
            updateSession = True
            sessionTimes = []
        else:
            command = input("Return to main menu? ")
            if command.lower() == "yes":
                mainMenu = True
                cubeTimer = False
                dataMenu = False
            else:
                pass

        while session == True:
            time.sleep(.1)
            result = cubeTime()
            time.sleep(.1)
            if result == None:
                command = input("You have paused the timer, would you like to save (save) or return to the main menu (menu)? ")
                if command.lower() == "save":
                    if updateSession == True:
                        appendSession(sessionTimes, fuzzyPath)
                        sessionTimes = []
                    else:    
                        savedData = storeSession(sessionTimes)
                        print(json.dumps(savedData, indent=2))
                        writeSession(savedData, destination)
                        sessionTimes = []
                elif command.lower() == "menu":
                    mainMenu = True
                    session = False
                    cubeTimer = False
                    dataMenu = False
            elif type(result) == float:
                sessionTimes.append(result)
                print(sessionTimes)
\$\endgroup\$
  • 1
    \$\begingroup\$ @Daniel I have put the code directly in the post and would be happy to explain my reasoning behind any portion of the program. \$\endgroup\$ – Jordan S. Jul 14 '18 at 19:43
  • \$\begingroup\$ Do you feel good about making implementation in object-oriented programming? Also, you have mentioned code needs further modularization. Do you expect advice for architecture? (for a single script it may be overkill) \$\endgroup\$ – Roman Susi Jul 15 '18 at 5:35
  • \$\begingroup\$ If you are ok with OOP, State design pattern may help make your code much less complex and organized, eg - en.wikipedia.org/wiki/State_pattern , sourcemaking.com/design_patterns/state/python/1 , \$\endgroup\$ – Roman Susi Jul 15 '18 at 5:44
  • 1
    \$\begingroup\$ @RomanSusi: That sounds like the start of an answer. \$\endgroup\$ – Graipher Jul 15 '18 at 8:29
3
\$\begingroup\$

You currently have createSessionStructure, storeSession, writeSession and appendSession. These all create or manipulate a Session object and therefore I would make them members of a class:

class Session:
    def __init__(self, times=None):
        self.data = {"session": {"timestamp": time.ctime(time.time()),
                                 "times": [] if times is None else times}}

    @classmethod
    def from_json(cls, filepath):
        with open(filepath, "r+") as f:
            session = cls()
            session.data = json.load(f)
            return session

    def extend(self, times):
        self.data["session"]["times"].extend(times)

    def save(self, filpath):
        with open(filepath, "w") as f:
            json.dump(self.data, f, indent=4)

I did not change the structure of the saved data, on purpose, so it is still compatible of what you have already. If not for that I would probably have taken out the top level dictionary and started with the inner dictionary. Maybe even both (see how I have to say session.timestamp = session.data["session"]["timestamp"], when it should actually just be sesstion.timestamp = data["timtestamp"]?).

If you don't care for your old data, then I would propose to use this instead (which I assume in the rest of this answer):

class Session:

    def __init__(self, times=None, timestamp=None):
        self.timestamp = time.ctime(
            time.time()) if timestamp is None else timestamp
        self.times = [] if times is None else times

    @classmethod
    def from_json(cls, filepath):
        with open(filepath, "r+") as f:
            return cls(**json.load(f))

    def save(self, filpath):
        with open(filepath, "w") as f:
            data = {"timestamp": self.timestamp,
                    "times": self.times}
            json.dump(data, f, indent=4)

    def __str__(self):
        out = [f"Session created at {self.timestamp}:"]
        out.extend(str(time) for time in self.times)
        return "\n".join(out)

    def average_time(self):
        return sum(self.times) / len(self.times)

This class also has two additional methods needed in the main loop, getting the average session time and printing a session using the magic method __str__.


Now, let's get to your main loop and how to use this class (not that different from your standalone functions). Currently you are using all kinds of flags, which are defined globally but only ever used locally to decide which part of the menu to run. This can be simplified a lot by defining separate functions that do some part of the menu and just return when done.

def choose_session():
    file_names = os.listdir("times/")
    for session in file_names:
        print(session)
    print("-" * 20 + "\n")
    file_name = None
    while file_name not in file_names:
        file_name = fuzzy_match(input("file name: "), file_names)
    file_path = os.path.join('times', file_name)
    return file_name, file_path


def view_session():
    print("Which session would you like to view?\n")
    print(("-" * 20) + "\n")
    file_name, file_path = choose_session()
    session = Session.from_json(file_path)
    print(f"Session {file_name}")
    print(session)
    command = input("Display average (av), or quit (quit): ")
    if command.lower() == "av":
        print(session.average_time())


def view_global():
    print("This area has not been implemented yet, returning to data menu.")


def data_menu():
    while True:
        command = input(
            "Would you like to view global (gl), session (ses) times, or go back (back)?").lower()
        if command == "gl":
            view_global()
        elif command == "ses":
            view_session()
        elif command == "back":
            return
        else:
            print("I don't understand that.")


def timer_menu():
    command = input(
        "Start a new session (new) or continue (cont) an old session? ").lower()
    if command == "new":
        session = Session()
        command = input("Do you want to name this file (yes/no)? ").lower()
        if command == "yes":
            name = input("Name: ")
        elif command == "no":
            name = session.timestamp.translate({ord(" "): "-", ord(":"): "."})
        file_path = os.path.join("times", name + ".json")
    elif command == "cont":
        print("Which session would you like to continue? ")
        _, file_path = choose_session()
        session = Session.from_json(file_path)
    elif input("Return to main menu? ").lower() == "yes":
        return

    while True:
        time.sleep(.1)
        result = cube_time()
        time.sleep(.1)
        if result is None:
            command = input(
                "You have paused the timer, would you like to save before returning to the main menu (yes/no)? ")
            if command.lower() == "yes":
                session.save(file_path)
            return
        else:
            session.times.append(result)
            print(session.times)
            print("Average:", session.average_time())


def main_menu():
    while True:
        print("Main Menu")
        print("Commands: times, timer, or quit")
        command = input("Where would you like to go: ").lower()
        if command == "times":
            data_menu()
        elif command == "timer":
            timer_menu()
        elif command == "quit":
            return
        else:
            print("I don't understand that.")

if __name__ == "__main__":
    destination = "C:/Coding/cube-timer/"
    os.chdir(destination)
    print("Welcome to Cube Timer!")
    print("======================")
    main_menu()

I also renamed cubeTime and fuzzyMatch to cube_time and fuzzy_match to conform to Python's official style-guide, PEP8.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for the awesome post! I hadn't even thought of using a class or using functions to control menu state. Do I have permission to use this code in my project? I'd give you credit in the source code itself and on Github. \$\endgroup\$ – Jordan S. Jul 15 '18 at 17:17
  • \$\begingroup\$ @JordanS.: Of course. All content posted on the Stack Exchange network by its users is automatically licensed with the CC BY-SA 3.0, attribution required, license. So feel free to use it. Adding a link to this answer (which you can get using the share link) together with my nickname should satisfy the attribution required clause. \$\endgroup\$ – Graipher Jul 15 '18 at 17:21
2
\$\begingroup\$

You seem to have heavily focused on the session part without taking proper care of the user experience of the core of your program: the timer.

One thing excrutiatingly missing is the ability to see the time running while the timer is on. Besides, timers for cube competition usually have an observation phase of 15 seconds, IIRC. You should have tried to make these part right before going into the full session save + load stuff.

First is the ability to show the time elapsed, while waiting for the user to press the space bar. For that you’ll need a thread responsible of the display while your main thread is blocked on the keyboard.wait(space) line. For somewhat accurate representation of time, this thread will store its own perf_counter (btw, kudos for not using time.time() here):

import threading


class CubeTimer(threading.Thread):
    def __init__(self, callback):
        super().__init__()
        self.callback = callback
        self.should_stop = threading.Event()

    def run(self):
        initial_time = time.perf_counter()
        while not self.should_stop.is_set():
            current_time = time.perf_counter() - initial_time
            self.callback(current_time)
            time.sleep(.05)

    def stop_timer(self):
        self.should_stop.set()

The time "stored" within this timer will be a little bit latter than when the user press the space bar due to resource management to start the thread, but the accuracy of the timer should be sufficient enough to show something while the timer is on.

You’ll also note that this thread take a callback to display the time elapsed instead of printing directly. This is to allow for more flexibility and stay generic enough in case you want to expand it for an other display (think GUI for instance).

The callback I’ll be using is:

def show_time(seconds):
    minutes = int(seconds) // 60
    print('{:02}:{:06.3f}'.format(minutes, seconds%60), end='\r')

This is as simple as it can get in terms of time formatting, but the nice trick is the end='\r' part where every successive print will overwrite the previous one, leaving the user thinking the timer is updating itself on screen.

Calling this is done by adapting your cubeTime() a bit:

def cube_time(callback):
    timer = CubeTimer(callback)

    keyboard.wait("space")
    start = time.perf_counter()
    timer.start()

    keyboard.wait("space")
    end = time.perf_counter()
    timer.stop_timer()
    timer.join()

    # These lines fix a bug where space characters would end up littering the menu after the timer closed
    # By pressing backspace it clears any characters in the terminal to ensure a better user experience
    keyboard.press_and_release("backspace")
    keyboard.press_and_release("backspace")

    return end - start


if __name__ == '__main__':
    print('Press space to start/stop the timer')
    print(cube_time(show_time))

Note that I use a perf_counter here as well and I update it directly when the space bar is pressed. This is our main source of time and should be more accurate than what the thread is displaying. You will also notice that the last print is also overwritting whatever intermediate display was in place, due to the previous end='\r'.

Now to include an observation phase, it is only a matter of extending the behaviour of CubeTimer and adding a third spacebar event in cube_time:

import time
import threading

import keyboard


class CubeTimer(threading.Thread):
    def __init__(self, callback):
        super().__init__()
        self.callback = callback
        self.observation = True
        self.initial_time = time.perf_counter()
        self.lock = threading.Lock()
        self.should_stop = threading.Event()

    def run(self):
        self.initial_time = time.perf_counter()
        while not self.should_stop.is_set():
            with self.lock:
                current_time = time.perf_counter() - self.initial_time
                if self.observation:
                    current_time -= 15
            self.callback(current_time)
            time.sleep(.05)

    def stop_observation(self):
        with self.lock:
            self.observation = False
            self.initial_time = time.perf_counter()

    def stop_timer(self):
        self.should_stop.set()


def show_time(seconds):
    sign = ' '
    if seconds < 0:
        sign = '-'
        seconds = -seconds
    minutes = int(seconds) // 60
    print('{}{:02}:{:06.3f}'.format(sign, minutes, seconds%60), end='\r')


def cube_time(callback):
    timer = CubeTimer(callback)

    keyboard.wait("space")
    observation = time.perf_counter()
    timer.start()

    keyboard.wait("space")
    start = time.perf_counter()
    timer.stop_observation()

    keyboard.wait("space")
    end = time.perf_counter()
    timer.stop_timer()
    timer.join()

    # These lines fix a bug where space characters would end up littering the menu after the timer closed
    # By pressing backspace it clears any characters in the terminal to ensure a better user experience
    keyboard.press_and_release("backspace")
    keyboard.press_and_release("backspace")
    keyboard.press_and_release("backspace")

    overshot = start - observation > 15
    return end - start, overshot


def session(callback=show_time):
    session = []
    while True:
        print('Press space to start and end the timer')

        time, overshot = cube_time(callback)
        if overshot:
            print(time, 'seconds + penalty')
        else:
            print(time, 'seconds')

        session.append(time)
        print(session)

        if input('Again? [Y/n] ').lower() not in ('', 'y', 'yes'):
            break


if __name__ == '__main__':
    session()

I included the start of a session management, but @Graipher's answer has already this part covered.

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