Code Review for Python project: Time Calculator

Tell us what’s happening:
Hi, I am new to Python programming. Just completed the challenge of developing a Time Calculator. All the unit tests passed but I would like to refactor my code. Kindly review my code and provide me feedback and suggestions for the same.

Thanks in advance,
Neha Yagnesh

Your code so far
https://repl.it/@nehaYagnesh/boilerplate-time-calculator

Your browser information:

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36.

Challenge: Time Calculator

Link to the challenge:

1 Like

Some things I’ve noticed and other pointers. Some of these are repeating in other places too. Keep in mind that these should be approached just as what they are - tips, pointers, suggestions what can be looked at and possibly improved. They may give you some other ideas what can be improved further along the way. Remember also to do small changes at a time and check often if everything is working properly after them.

  • At the beginning of function value is assigned to new_time , but later that variable isn’t used, rather than that variable name is overwritten with new assignment.

  • Regarding line:

time,meridian = starttime.split()

time is name of the build-in module. It’s best to avoid using names that are shadowing names build-in modules or functions. However if such name really would be the best, then underscore can be added at the end of it, like time_, to kind of still keep the name and don’t overwrite build-in module name.

  • Regarding lines:
startminutes_hours =  ''.join('{:.2f}').format(int(minutes) / 60)
starttime_hours = float(startminutes_hours) + int(hours)

I’m not entirely sure what is supposed to be happening here, but it kind of looks that float number is converted to str just to be converted back to float in the next line. Is this just an unconventional way to round number? Why not use round function when minutes are divided, with optional argument specifying number of digits precision for it? startminutes_hours isn’t used anywhere where it might need to be str. It’s not used anywhere else actually, so one step further might be removing that line and move minutes division and rounding it to the following line.

  • Repeated type conversions

int(minutes), int(hours), int(totaltime_hours), str(totaltime_minutes), etc. - try to minimize number of places where type needs conversion. Taking as example minutes and hours, there’s no reason to keep them as str when defined and then converting to int whenever they are needed, just convert them to int when being defined and use that later.

  • Regarding lines:
if dayoftheweek is not None:
elif dayoftheweek is None and number_of_days == 1:
elif dayoftheweek is None and number_of_days > 1:
else:

Notice that in here if first condition catches dayoftheweek is not None, then in other possible cases dayoftheweek have to be None and such condition can be safely removed from elifs leaving there only condition for number_of_days.

  • Staying in this part of the code, take a look at what is happening with new_time.
new_time = str(total_hour_12format) + ':' + str(totaltime_minutes)+ ' '+ str(meridian_l[0]) + ', ' + days_in_week[day_index % 7] + ' (' + str(number_of_days) + ' days later)'
new_time = str(total_hour_12format) + ':' + str(totaltime_minutes)+ ' '+ str(meridian_l[0]) + ', ' + (days_in_week[day_index % 7]) + ' (next day)'
new_time = str(total_hour_12format) + ':' + str(totaltime_minutes)+ ' '+ str(meridian_l[0]) + ', ' + days_in_week[day_index % 7]
new_time = str(total_hour_12format) + ':' + str(totaltime_minutes)+ ' '+ str(meridian_l[0]) + ' (next day)'
new_time = str(total_hour_12format) + ':' + str(totaltime_minutes)+ ' '+ str(meridian_l[0]) + ' (' + str(number_of_days) + ' days later)'
new_time = str(total_hour_12format) + ':' + str(totaltime_minutes)+ ' '+ str(meridian_l[0])

Notice that part of the assignment is always the same, this part can be placed before of the conditional block, with adding just the changing part in the conditional block (or in two blocks).

  • Regarding line:
elif int(totaltime_hours) >= 12 and int(totaltime_hours) < 24:

Python have a nice way to chain and simplify such conditions referring to the same variable, i.e. a >= 1 and a < 3 can be written as: 1 <= a < 3

  • Comments

They are often saying exactly what is happening in the code. That’s usually clear after reading the code and not really needed. What would be better is to include in comments the reason why something is happening. Taking for example:

while int(totaltime_hours) >= 24:     # If the totaltimehours is greater then 24 then subtract by 24
        totaltime_hours = int(totaltime_hours) - 24

Comment here is just repeating the code as such it’s not needed and could be removed. If instead comment would say why it’s looping and subtracting from totaltime_hours, that would make comment beneficial for understanding the code.

  • Style guide

Take a look at PEP 8 style guide for python, especially regarding way of naming variables and maximum length of line. It defines some good practices to keep code readable and consistent with how usually others format code written in python.

1 Like

Hi, first of all: great work! I might not be the one to comment your work, since I am a beginner in programming in general, so 1 question for me to learn from:
You are assigning zeroes and empty variables at the start of your code, and these are used furhter down the program. Is this the proper way to do it? (Again, newbie here…I am assigning them as they turn up in later lines of code and I think your way is more clear, but I can also imagine that a person reading the code might not want scroll all the way up to see what the variable was?)

In line 21 and 22 there is lots of rounding to 2 digits :

   # Converting the duration into hours
   duration_minutes_hours  = ''.join('{:.2f}').format(int(add_minutes) / 60)
   duration_add_hours = float(''.join('{:.2f}').format(float(duration_minutes_hours)))+ float(''.join('{:.2f}').format(float(add_hours)))

Is this really necessary? In this case, it probably will not affect the precision of the outcome, but I can imagine in more complex calculations rounding during calculations can result in an incorrect outcome, so my idea is to prevent that as a general habit.
Furthermore, a string is converted into an integer, but when calculating, it is roudend to 2 digits. Somehow that seems contradictory. Why not convert the string into a float?

(If you’re interested: please find my code at https://repl.it/@Brain150/FCC-Python-Time-Calculator#time_calculator.py
I tried to see this a an exercise in calculation, so I went to town with floor divisions and modulus :wink: . Do comment if you see improvements!

1 Like

Hey! Thank you for reviewing and providing such descriptive feedback. I have implemented all the suggestion provided by you and my code looks much better and stable. Once again I appreciate your time and effort. You helped me learn something new which is really cool :innocent:

1 Like

Hey Brian,

Thanks for the feedback. I have now removed the unnecessary type conversion from my code.

I had a look at your code and found it pretty good. You made good use of floor division and modulus which I will definitely give a try.

For this block of code:

#split start time into time and period
	start_part = start.split(" ")
	start_time = start_part[0]
	start_period = start_part[1]

you can do this in a single step:

start_time, start_period = start.split()

As recommended by @sanity you can have a look at the PEP 8 style guide for python. I found it really beneficial.

Happy Coding!

1 Like

Definitely various places are improved. As this can be slightly continuous process and while improving parts of the code, other places, where something can be done, can reveal themselves I’ve taken another look.

Some pointers, tips and suggestions.

  • Regarding line:
total_hour_12format = 0

This variable assigned at the beginning is not used later for anything. Other occurrences of variable with this name in code are after the name is re-assigned.

  • Regarding lines:
    total_time_24format_decimal = ''.join('{:.2f}')        \
                                    .format(starttime_hours 
                                            + duration_add_hours)

Just now I’ve noticed that here’s happening even more. Notice however that join is obsolete here, it joins elements in iterable ('{:.2f}') using '', effectively ending up with the same string. And '{:.2f}' is already a string, for which format method can be called.

  • Regarding lines:
        if (meridian in meridian_total_list 
                and (totaltime_hours - hours == 0 
                        and number_of_days == 1)):
        elif (meridian in meridian_total_list 
                 and (abs(totaltime_hours - hours) > 12 
                         or number_of_days == 1)):

Is it even possible that meridian is not in meridian_total_list? Either way conditions here are somewhat complicated and partially repeat. What might be done about that is adding variables for repeating parts of this expression, which will be evaluated just before the if/else block. I.e. meridian in meridian_total_list and totaltime_hours - hours could be treated like that. While cutting down like that on the number of operations will not matter much in this example in terms of efficiency, it will still allow for some clean up for readability.

It should be also possible to move this if/else block outside of while loop and join it with:

if abs(totaltime_hours - hours) < 12:
            meridian_total_list.remove(meridian)
            meridian_l = meridian_total_list

from the block before, to simplify as well.

  • Taking a look again at the last if/else block and what is happening with new_time
new_time += (', ' 
    + days_in_week[day_index % 7] 
    + ' (' 
    + str(number_of_days)
    + ' days later)')
new_time += (', ' 
    + days_in_week[day_index % 7] 
    + ' (next day)')
new_time += (', ' 
    + days_in_week[day_index % 7])
new_time += ' (next day)'
new_time += (' ('
    + str(number_of_days) 
    + ' days later)')

Once more notice that part here is repeating. Or rather there are two separate paths that do not depend on each other and can be considered separately - condition for dayoftheweek is not None and conditions for number_of_days. They can be split to two separate if/else blocks, what will make it even more clear.

  • String concatenation

While using the + operator for incidental concatenating doesn’t look too complicated:

totaltime_mins = '0' + str(totaltime_mins) 

once it get to the point of looking like:

    new_time = (str(total_hour_12format) 
                + ':' 
                + str(totaltime_mins)
                + ' '
                + str(meridian_l[0]))

it can be harder to read. Take a look at the mentioned before format method of the str and the so-called f-strings, they could improve these parts.

1 Like

This code is already outside while loop. I didn’t get this part.

I have tried to follow the suggestions you recommended: https://repl.it/@nehaYagnesh/boilerplate-time-calculator#time_calculator.py

What I meant is that code, along with the code in while loop setting meridian_l could probably be put together in it own place. Rather than being in while loop, or when time is changed to 12 hours format.

Regarding lines:

    if number_of_days > 1:
        if dayoftheweek is not None:
            new_time += f', {days_in_week[day_index % 7]} ({number_of_days} days later)'
        else:
            new_time += f' ({number_of_days} days later)'
    elif number_of_days == 1:
        if dayoftheweek is not None:
            new_time += f', {days_in_week[day_index % 7]} (next day)'
        else:
            new_time += ' (next day)'
    elif dayoftheweek is not None:
        new_time += f', {days_in_week[day_index % 7]}'

There’s still at least one step that can be done here. Notice that string added when dayoftheweek is not None doesn’t change depending on number_of_days. Both are adding their own parts, this means they can be separated to two if/else clauses, without one being nested in another.

Thanks a lot, I made a note to make the split more efficient, so this tip is more than welcome :slight_smile: