1
\$\begingroup\$

How to improve this code, how to make it accessible to get user feedback?

This is a minimal clock time validation app. There is a clock face and the user is asked to validate the time the clock hands show. It is written in Python with Turtle and Tkinter. This is the first app I made. I tried to compose the code so that it is minimal and clean. I tried to keep functions and the operations of the app separate. I created a config dictionary for default setups with constants, and a state for variables that change during the running of the program. I tried to create functions that do one thing. I installed a linter and formatted the code. I formatted the import statements so that only specific functions are imported.

I understand that it can be valuable to create a minimal viable product with only one feature, and then find an audience and get feedback. I don't know if there is a need for such an app, I find it very useful and fun. Among other things, apart from learning to read the time easier, I find that this app helps me with number sense, visualising proportions and understanding mathematical connections. It is also fun and motivating to develop an app that I enjoy using. Developing it also brought ideas for other minimal viable products, like fraction validation with different patterns.

It is inspired by Barbara Arrowsmith Young's work. She devised a cognitive clock reading exercise, with many clock hands, to improve the connectivity and functioning of her brain. I don't have access to the actual clock she made and don't know if it is very similar.

from math import radians, sin, cos
from random import randint
from datetime import datetime
from tkinter import Tk, END, Canvas, Frame, Entry, Button, Label
from turtle import TurtleScreen, RawTurtle

config = {
    "screen": {
        "title": "Clock Validation",
        "canvas_width": 600,
        "canvas_height": 600,
    },
    "navigation": {
        "title": "Navigation Window",
        "label_text": "Validate time value (HH:MM:SS)",
    },
    "theme": {
        "light_bg": "white",
        "dark_bg": "black",
    },
    "hands": {
        "hour": {
            "shape": "arrow",
            "color": "blue",
            "size": (1, 10),
        },
        "minute": {
            "shape": "arrow",
            "color": "red",
            "size": (1, 15),
        },
        "second": {
            "shape": "arrow",
            "color": "orange",
            "size": (1, 20),
        },
    },
    "markings": {
        "radius": 220,
        "number_of_markings": 60,
        "hours": {
            "color": "blue",
            "size": 10,
        },
        "minutes": {
            "color": "green",
            "size": 5,
        },
    },
    "buttons": {
        "validate": {
            "text": "Validate Time",
        },
        "next": {
            "text": "Next Clock",
        },
        "theme": {
            "dark_text": "🌘",
            "light_text": "☀️",
        },
    },
}

state = {
    "theme": {
        "light_mode": False,
        "theme_bg": config["theme"]["dark_bg"],
    },
    "current_time": {
        "hours": datetime.now().hour,
        "minute": datetime.now().minute,
        "seconds": datetime.now().second,
    },
}

# VALIDATION LOGIC


# Validation handler
def handle_validation():
    user_time = input_field.get()
    valid_time, message = validate_time(user_time, state["current_time"])
    validation_label.config(text=message)


# Function to validate the user's input
def validate_time(user_time, state):
    current_hour, current_minute, current_second = state.values()

    try:
        user_hour, user_minute, user_second = map(int, user_time.split(":"))
    except ValueError:
        return False, "Invalid format (HH:MM:SS)"

    if not (0 <= user_hour <= 24 and 0 <= user_minute <= 59 and 0 <= user_second <= 59):
        return False, "Invalid time values"
    elif (
        # User input hours are correct
        (
            user_hour == current_hour
            or user_hour - 12 == current_hour
            or user_hour == current_hour - 12
            or (user_hour in [0, 24, 12] and current_hour == 0)
        )
        and
        # User input minutes and seconds are correct
        user_minute == current_minute
        and user_second == current_second
    ):
        return True, "Awesomeness, keep up the good work!"
    else:
        return False, "Oops, try again!"


# FUNCTIONS FOR DRAWING THE CLOCK


# Functions to create angles from clock values
def hours_angle(hours, minutes):
    return (hours % 12) * 360 / 12 + minutes * 360 / (12 * 60)


def minutes_angle(minutes):
    return minutes * 360 / 60


def seconds_angle(seconds):
    return seconds * 360 / 60


# Loads current time values
def load_current_time():
    hours, minutes, seconds = state["current_time"].values()
    return [hours, minutes, seconds]


# Calculates hands angles
def calculate_angles(current_time):
    hours, minutes, seconds = current_time
    hour_angle = hours_angle(hours, minutes)
    minute_angle = minutes_angle(minutes)
    second_angle = seconds_angle(seconds)
    hands_angles = [hour_angle, minute_angle, second_angle]
    return hands_angles


# Function for drawing the hands
def draw_hands():
    current_time = load_current_time()
    hands_angles = calculate_angles(current_time)
    for hand, angle in zip(reversed(clock_hands), reversed(hands_angles)):
        # Set the hands' angles
        hand.setheading(90 - angle)


# Generates random hours, minutes, and seconds
def generate_random_time_values():
    random_hours = randint(0, 11)
    random_minutes = randint(0, 59)
    random_seconds = randint(0, 59)
    return [random_hours, random_minutes, random_seconds]


# Refreshing state with new time values
def refresh_time(state):
    refreshed_state = state
    random_time = generate_random_time_values()
    refreshed_state = {
        "hours": random_time[0],
        "minutes": random_time[1],
        "seconds": random_time[2],
    }
    return refreshed_state


# Next clock drawing handler
def load_next_clock():
    # Clear the input field and validation message
    input_field.delete(0, END)
    validation_label.config(text="")

    # Refresh state with new time values
    state["current_time"] = refresh_time(state["current_time"])

    # Change direction of the hand  turtles
    draw_hands()


# GUI SETUP


def toggle_theme():
    # If it is dark mode at the moment, make light
    if state["theme"]["light_mode"] is False:
        state["theme"]["light_mode"] = True
        state["theme"]["theme_bg"] = config["theme"]["light_bg"]
        screen.bgcolor(config["theme"]["light_bg"])
        theme_button.config(text=config["buttons"]["theme"]["dark_text"])
    # If it is light mode, make dark
    else:
        state["theme"]["light_mode"] = False
        state["theme"]["theme_bg"] = config["theme"]["dark_bg"]
        screen.bgcolor(config["theme"]["dark_bg"])
        theme_button.config(text=config["buttons"]["theme"]["light_text"])


# Create tkinter GUI for interaction
root = Tk()
root.title(config["screen"]["title"])

# Create a Canvas to hold the Turtle screen
canvas = Canvas(
    root,
    width=config["screen"]["canvas_width"],
    height=config["screen"]["canvas_height"],
)
canvas.pack()

# Create a Frame to hold the button
button_frame = Frame(root)
button_frame.pack(side="bottom", fill="both", expand=True)

# Create a Turtle screen within the canvas
screen = TurtleScreen(canvas)
screen.bgcolor(state["theme"]["theme_bg"])

# Create the input field and buttons
input_field = Entry(button_frame)
input_field.pack()

# Create validate button
validate_button = Button(
    button_frame, text=config["buttons"]["validate"]["text"], command=handle_validation
)
validate_button.pack()

# Create validation label
validation_label = Label(button_frame, text=config["navigation"]["label_text"])
validation_label.pack()

# Create next button
next_clock_button = Button(
    button_frame, text=config["buttons"]["next"]["text"], command=load_next_clock
)
next_clock_button.pack()

# Create theme toggle:
theme_button = Button(
    button_frame, text=config["buttons"]["theme"]["light_text"], command=toggle_theme
)
theme_button.pack()


# DRAWING THE MARKINGS AND THE HANDS

# Create a turtle to draw the clock face
clock = RawTurtle(screen)
clock.speed(100)

# Make turtles for clock hands
clock_hands = []
for hand_name, hand_config in config["hands"].items():
    hand = RawTurtle(screen)
    hand.shape(hand_config["shape"])
    hand.color(hand_config["color"])
    hand.shapesize(
        stretch_wid=hand_config["size"][0], stretch_len=hand_config["size"][1]
    )
    clock_hands.append(hand)

# Load values for the markings
clock_radius = config["markings"]["radius"]
number_of_markings = config["markings"]["number_of_markings"]
hours_dot_size = config["markings"]["hours"]["size"]
hours_dot_color = config["markings"]["hours"]["color"]
minutes_dot_size = config["markings"]["minutes"]["size"]
minutes_dot_color = config["markings"]["minutes"]["color"]

# Draw markings
for i in range(number_of_markings):
    angle = radians(i * 360 / number_of_markings)
    x = clock_radius * sin(angle)
    y = -clock_radius * cos(angle)

    if i % 5 == 0:
        # Larger dot for hours
        clock.penup()
        clock.goto(x, y)
        clock.dot(hours_dot_size, hours_dot_color)
    elif i % 1 == 0:
        # Smaller dot for minutes
        clock.penup()
        clock.goto(x, y)
        clock.dot(minutes_dot_size, minutes_dot_color)

# Draw hands
draw_hands()

# Start the tkinter main loop to manage the GUI
root.mainloop()
```
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Thank you for the obvious effort put into readability.

isort

Your imports are nice enough as is. But you're working too hard. Run "$ isort *.py" and be done with it, don't give it a second thought. The package's motto is "I sort ... so you don't have to."


config

Gosh, that's a lot of config details! Reading this for the first time, I'm wondering if maybe it's not the most convenient way to achieve your goal. There's lots of key names that appear here as str and will wind up being bound to identifiers down in the code. Feel free to keep it as-is, but I will toss out a pair of alternatives to consider. And the three Hands makes me wonder if we'd like a class or a named tuple to represent them.

This dict may look like data but really it's code, within a source file. Consider using json.dump(), or better, yaml.dump(), to serialize it out to a config file. Use the same git source control edit / commit practices on the config file that you use on your *.py source files.

Consider using keyword arguments (kwargs) in a function's signature to adjust some of these parameters. And then typer gives you CLI --help "for free".

def main(
        nav_title="Navigation Window",
        nav_label_text="Validate time value (HH:MM:SS)",
        theme_light_bg="white",
        theme_dark_bg="black",   # and so on...
    ):
    ...

if __name__ == "__main__":
    typer.run(main)

comments

There's nothing wrong with writing comments -- feel free to do so.

But sometimes, if you feel a need to explain code which isn't sufficiently clear, the comment is a code smell that is helping you to identify a refactor.

# VALIDATION LOGIC


# Validation handler
def handle_validation():
    ...

# Function to validate the user's input
def validate_time(user_time, ...

All of those identifiers are great.

The "validation logic" comment is maybe helpful for grouping things, maybe not too redundant with the "valid"s we see in function names. Or maybe it would be helpful to banish these functions to a new validation.py module, and import them. Similarly for a draw_clock.py module.

The next two comments are on the redundant side, enough that I'd probably delete one or both of them. If you do wish to retain descriptions of those (well named, self explanatory!) functions, then use a """docstring""" rather than a # comment. A single English sentence would likely suffice.

It would be very nice if you explain we're expecting str input rather than a datetime.time:

def validate_time(user_time: str, ...

Now, about that second argument, the state dict. Consider creating a class so you can turn this function into a method.

    def validate_time(self, user_time: str):

Then we might access self.state. Or we might break out its values to store each one directly as an object attribute.

I confess that referencing state.values() confuses me a bit. I was expecting to see state.current_time.values().

Your H,M,S tuple unpack makes me slightly nervous, as it's reading from a dict rather than a tuple. Yes, in cPython since 3.5 we've had stable iterator order which matches the key insertion order. But the python language is bigger than cPython, there are other interpreters. It just seems slightly jarring that we don't have three dict de-refs there. Feel free to keep it, anyway. Consider turning H,M,S into a namedtuple or dataclass. Or better, prefer to use a time.

BTW, kudos on that map(int, user_time.split(":")) tuple unpack expression. It's a very clear, natural way to express your intent.

Give the signature a type annotation, please, so caller knows to expect a tuple[bool, str].


diagnostic message

        return False, "Invalid format (HH:MM:SS)"

This is nice as-is. Consider tacking on the bad user_time value, to help some future maintenance engineer diagnose bugs quicker.

Rather than returning a boolean, you possibly would like to raise ValueError(...), so caller can't accidentally ignore the False value.


half-open interval

Kudos for the very clear math notation in this expression:

    if not (... and 0 <= user_second <= 59):

You might want to habitually prefer a half-open over a closed interval: 0 <= user_second < 60 There are several advantages.

  1. They compose nicely, e.g. Mon .. Wed combined with Wed .. Sat gives all weekdays.
  2. They match the builtin range semantics.
  3. Magic numbers appear in their more obvious form, e.g. 59 vs 60.
  4. Continuous and discrete work the same (float and int, also datetime and date).

nit: Here's a fun fact. The <= 59 expression isn't exactly correct. Twice a year (start of January & July) a leap second may be inserted. Which lets 23:59:59 tick over to 23:59:60 before advancing to 00:00:00. Many people (well, non-astronomer people) ignore this, and you should, too. The takeaway is "timekeeping is complex", and you'd be better off modeling instants of time with a mature library such as datetime.


modulo

    elif ( ...
        (
            user_hour == current_hour
            or user_hour - 12 == current_hour
            or user_hour == current_hour - 12
            or (user_hour in [0, 24, 12] and current_hour == 0)
        )

Yikes! Such a simple concept, so much complexity. It looks like you had trouble with edge cases and just kept piling one fix on top of another.

Step back a moment. The math concept you're looking for is % modulo, which grade school math teachers often introduce in terms of clock hands. Compare both quantities mod 12 and we're done.

If datetime times were involved we wouldn't be jumping through hoops like this.

Maybe we're sad that we lost the AM/PM indicator bit? Better, maybe we should have immediately trimmed twelve hours off any PM input values, before it could even get here.


@property

The FOO_angle functions are very nice, keep them as-is. Perhaps mark them as _private helpers with a leading _ underscore. In general, your Public API is pretty big, exporting a great many identifiers.

I will just point out that if you create your own class (maybe a dataclass) to represent an instant in time, there is an opportunity to turn those functions into three @property decorated methods.


negation

def toggle_theme could toggle by assigning not state["theme"]["light_mode"].

Notice that we could construct the relevant key name with an expression like:

>>> ['dark', 'light'][False]
'dark'
>>> ['dark', 'light'][True]
'light'

main guard

root = Tk()

This is bog standard, it's nice as-is.

Please bury it within def main(): or another function. That way other modules, such as a test suite, can import this one and exercise the various helper functions.

It is usual that import should not print, create a new window, or have other annoying side effects, beyond defining functions and constants.

You have a bunch of helpers. They would benefit from an automated unit test suite.


unyt

There's no need to adopt this library. But I will note in passing that unyt lets you annotate your variables with degree or radian. This can promote type safety and reduce confusion.


This well-engineered application achieves its design goals.

I would be willing to delegate or accept maintenance tasks on it.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you @J_H for this detailed review! I am grateful. \$\endgroup\$ Commented Oct 31, 2023 at 7:17

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.