Hi everybody! Hope you’re staying safe during this Covid mess! I was hoping someone could check out my 2nd project in Python and let me know what they think. Any tips, suggestions, ideas are greatly appreciated.
Link to GitHub for code - the code for review is in “time_calculator.py”:
Thanks so much! 
Challenge: Time Calculator
Link to the challenge:
Hi zaneaw, well done! Congratulations on finishing the project!
I very much like the readability of your code, the variables are clearly named and your comments explain well what is going on.
That said, there are quite a few places where you could simplify your code.
One thing you could do is use python’s feature of assigning multiple variables in one statement, so the lines
start = start.split()
meridiem = start[1]
meridiem = meridiem.upper()
time = start[0]
could be shortened to this:
time, meridiem = start.split()
meridiem = meridiem.upper()
You’re using rstrip on the input strings, which is good in general to improve robustness on inputs, but it’s not needed for the given test inputs.
In this line
new_day = int(new_hour / 24) # Return a whole number
you’re using a typecast for rounding, which gives the right result but is not the cleanest way to do it. Python has a special operator for floor division that you could use instead:
new_day = new_hour // 24 # Return a whole number
In the computation for the new weekday, you could save a lot of lines using the modulo operator. You’re already using it once here:
new_day = new_day % 7
Reusing it in the following if-elif part makes the code a lot shorter, so all of this
# Return statement with optional arg and next day
elif new_day == 1:
day_index += 1
day = days_of_week[day_index]
new_time = f"{new_hour}:{new_minute} {new_meridiem}, {day} {day_statement}"
# Logic / Return statement if more than next day and cutting to the next week
elif new_day > 1:
if new_day + day_index >= 7:
new_day = day_index - new_day
day_index = 0
while new_day > 0:
new_day -= 1
day_index += 1
day = days_of_week[day_index]
new_time = f"{new_hour}:{new_minute} {new_meridiem}, {day} {day_statement}"
can be replaced with this:
# Return statement with optional arg and next day
else:
day = days_of_week[(day_index + new_day) % 7]
new_time = f"{new_hour}:{new_minute} {new_meridiem}, {day} {day_statement}"
In this previous section, you are also currently overwriting the day variable, which means when you use it again in line 108, it does not have the original value passed to the function. While this doesn’t have any effect in this specific case, you should generally avoid overwriting the input argument variable as it makes the code less understandable and if you need to use the original value later, it’s not available anymore.
I saw you initialize a variable day_of_week that you don’t use anywhere, so I suggest you replace day with day_of_week here.
Finally, as the readme says there is only one optional argument to the function, namely the day of the week, you could define the function with exactly one optional default argument instead of an arbitrary number of arguments:
def add_time(start, duration, day=None):
Then you could not only safely remove this line:
day = day[0]
but also this complete part:
# Return statement, if it progresses by a day, add the day_statement
# If *day is included, everything dont above, else complete below
if day:
return new_time
else:
if day_statement == "":
new_time = f"{new_hour}:{new_minute} {new_meridiem}"
else:
new_time = (
f"{new_hour}:{new_minute} {new_meridiem} {day_statement}"
)
You don’t need this anymore because day will be None when not passed as argument, so the evaluation in the f-string (lines 89/94/104) will just add nothing to the new_time string.
Making more use of this behaviour, you can once again shorten the section for the new_day calculation by adding a spaces and comma to the day_statement and day strings:
if new_day == 1:
day_statement = " (next day)"
if new_day > 1:
day_statement = f" ({new_day} days later)"
With this,
if day:
# Take the day of the week out of the optional argument and capitalize
day = day.capitalize()
# Match the day to list of days_of_week and pull out the index
day_index = days_of_week.index(day)
# Total # of days is calulcated above and added into day_statement
# This allows us to cut down on loop or forgo loop while period
new_day = new_day % 7
# Return statement with optional arg and same day
if new_day == 0:
new_time = f"{new_hour}:{new_minute} {new_meridiem}, {day}"
# Return statement with optional arg and next day
else:
day_of_week = days_of_week[(day_index + new_day) % 7]
new_time = f"{new_hour}:{new_minute} {new_meridiem}, {day_of_week} {day_statement}"
is equivalent to
if day:
# Take the day of the week out of the optional argument and capitalize
day = day.capitalize()
# Match the day to list of days_of_week and pull out the index
day_index = days_of_week.index(day)
new_day = new_day % 7
day_of_week = f", {days_of_week[(day_index + new_day) % 7]}"
new_time = f"{new_hour}:{new_minute} {new_meridiem}{day_of_week}{day_statement}"
I hope my suggestions are clear, if not please don’t hesitate to ask!
And just in case you’re interested, here’s my own solution for the challenge:
https://repl.it/@Seventreesofwis/boilerplate-time-calculator#time_calculator.py