16
\$\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, "°")
Peilonrayz
44.4k7 gold badges80 silver badges157 bronze badges
asked Apr 7, 2020 at 14:12
\$\endgroup\$
0

2 Answers 2

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.

answered Apr 7, 2020 at 14:35
\$\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.

answered Apr 7, 2020 at 15:39
\$\endgroup\$
1
  • 3
    \$\begingroup\$ 0[0-9]|1[0-9] can be simplified to [0-1][0-9]. \$\endgroup\$ Commented Apr 9, 2020 at 15:02

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.