15
\$\begingroup\$

I'm a novice programmer. I started with Java years back and now mostly use VBA to automate tasks for my internships. I want to graduate from VBA and get into Python/R to properly automate tasks. The below code is my first Python project. I would like any of you to point out how it can be improved.

The program is meant to take in a time in HH:MM format and print out at what degrees (from 12:00) each of the hands will be at. This was adapted from a common finance interview brainteaser guide.

I'm thinking that if I get some good suggestions for this code I'll go ahead and make one that includes a second hand.

input_time = input("Please input a time (HH:MM)")

time = input_time.split(":")

clock_degrees = 360
clock_hours = 12
clock_minutes = 60
clock_degreesperhour = clock_degrees/clock_hours
clock_degreesperminute = clock_degrees/clock_minutes

miunte_hand_degrees = int(time[1])*clock_degreesperminute

if (time[0] == "12"):
    time[0] = "0"

hour_hand_degrees = (int(time[0])*clock_degreesperhour)+((int(time[1])/clock_minutes)*clock_degreesperhour)

print("When the time is", input_time, 'the hour hand is at', hour_hand_degrees, "° and the minute hand is at", miunte_hand_degrees, "°")
\$\endgroup\$
0
18
\$\begingroup\$

Currently your code has user input, calculation and output all mixed up. This is OK as a first step (your code works, yay!), but the next step should be separating the calculation from the IO part. For this a simple function which takes the input already as parsed numbers will do:

def clock_angles(hour, minute, second):
    """Convert the time in hour, minute, second into angles of clock hands
       on a 12 hour clock, in degrees."""
    if hour >= 12:
        hour -= 12
    assert 0 <= hour < 12 and 0 <= minute < 60 and 0 <= second < 60
    degrees_per_hour = 360 / 12
    degrees_per_minute = degrees_per_second = 360 / 60
    return (hour * degrees_per_hour,
            minute * degrees_per_minute,
            second * degrees_per_second)

It even has a docstring describing what it does and some simple input validation.

Your IO can then be wrapped under a if __name__ == "__main__": guard which ensures it is not being run when you import from this script from another script. You can also use a f-string to make formatting the output slightly easier.

if __name__ == "__main__":
    input_time = input("Please input a time (HH:MM:SS): ")
    hour, minute, second = map(int, input_time.split(":"))

    hour_angle, minute_angle, second_angle = clock_angles(hour, minute, second)
    print(f"When the time is {input_time} the hour hand is at {hour_angle}°, the minute hand is at {minute_angle}° and the second hand is at {second_angle}°.")

I included the second hand here. Being able to parse the input without the seconds is left as an exercise.

\$\endgroup\$
10
\$\begingroup\$

One thing that is missing is validation of user input.

If you omit the colon you get this: IndexError: list index out of range. If you enter an invalid time like 25:99 you obviously get inconsistent results. There is no point trying to process invalid input.

Fix: validate the input with a regular expression and capture values to hour and minute variables.

Then, reuse those variables, rather than repeating time[0], time[1]. Type casting (int) should then be done at the source as well. Don't repeat unnecessary operations.

Proposed code:

import sys
import re

input_time = input("Please input a time (HH:MM): ")

m = re.search("^(0[0-9]|1[0-9]|2[0-3]):?([0-5][0-9])$", input_time)
if m:
    hours = m.group(1)
    minutes = m.group(2)
else:
    print("Invalid time, try again")
    sys.exit(0)

# verification
print(f"Hours: {hours}, minutes: {minutes}")

To improve user experience, I have made the colon optional. Thus, 2359 will be treated like 23:59. This regex expects a leading zero in both minutes and hours, but you could change that. However if you decide the allow single digits, you will have to reintroduce the separator to avoid the ambiguity, because 01:08 can be expressed as 1:8 or 1:08 - without a separator you can't reliably tell hours from minutes.

Regular expressions are a complex subject and a full explanation is beyond the scope of this review. In short this expression validates a time in the range 00:00 to 23:59, with or without colon separator. The parentheses delimit the capture groups while the question mark indicates that the preceding symbol (:) is optional.

Note that there is a typo: miunte_hand_degrees. The danger would be that you assign another variable elsewhere in the code with the proper spelling (minute_hand_degrees) and you end up with two variables that are assigned different values, and you have introduced a bug in your program.

I have not verified the algo.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ 0[0-9]|1[0-9] can be simplified to [0-1][0-9]. \$\endgroup\$
    – Reinderien
    Apr 9 '20 at 15:02

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.