Python code Review

Project Feedback
#1

Hello Folks,

I wrote this basic python program which is a time calculator

https://repl.it/@mootarek/timecalculator

In this python challenge:

https://www.freecodecamp.org/learn/scientific-computing-with-python/scientific-computing-with-python-projects/time-calculator[https://www.freecodecamp.org/learn/scientific-computing-with-python/scientific-computing-with-python-projects/time-calculator]

I started learning python recently so I am not good at it, but I knew other languages before it, so It was kinda easy.

I am asking that some python fluent review this code (link above in the repl) and tell me what is bad about it, how can I improve my code in python as I am planning to code a big python project, also tell me the good parts in the code (if any) but I think it’s a bit modular bec. there’s a separate function for nearly anything.

Thanks, Guys

Time Calculator (Help! Make code cleaner)
#2

You are using lot of short function which are not reused also…
You should review your logic in some cases

  • image
    You can do above thing simply by num%7, and no need of function
  • image

You can implement above thing by using if elif only

  • image

use rjust function for implementing this

  • image

this if is unnecessary as, form is either “AM” or “PM”., condition is always true, hence unnecessary comparison…

I know you’ve put lot of efforts and tried to solve without searching for additional help, by implementing your thinking as it is. But you should always follow best practices and try to reduce execution time.
Try to reduce use of loops and additional unnecessary variables wherever possible.

Good job, you’ve completed your assignment, but now work on make it more efficient :slight_smile: :+1:

2 Likes
#3

Thanks a lot for this detailed review. yes, I think the code needs to be Improved. It’s a basic program that just outputs the time after some addition, and I made it unreadable and inefficient. I’ll work on making it cleaner and will also reuse some code.

#4

I think this is better no aimless pure functions bouncing here and there:

listofdays = [
    "sunday",
    "monday",
    "tuesday",  
    "wednesday", 
    "thursday",   # 6 + 12 = 18 formula
    "friday",       
    "saturday", 
]

def get_days(tothrs, day, form):    
    numdays = 0
    dayreturned = ''
    if day == '': 
        while tothrs >= 24:
          tothrs = tothrs - 24
          numdays = numdays + 1 

        numdays = (numdays + 1 if tothrs >= 12 and form == 'PM'
        else numdays)

        if numdays == 1:
            dayreturned = "next day"
        elif  numdays > 1:
            dayreturned =  f"{numdays} days later"  
        return tothrs if tothrs != 0 else 12, dayreturned
    else:
        dayreturned = ''
        indexofday = listofdays.index(day)

        while tothrs >= 24:
          tothrs = tothrs - 24
          numdays = numdays + 1

        numdays = (numdays + 1 if tothrs >= 12 and form == 'PM' 
        else numdays)

        if (numdays + indexofday) >= len(listofdays) :
            # number = len(listofdays) + numdays + indexofday
            temp = numdays
            numdays = numdays % 7
            dayreturned = (listofdays[numdays] + ' next day' 
            if temp == 1 
            else listofdays[numdays] +  f" ({temp} days later)")

        elif numdays == 0 :
            dayreturned = day

        elif numdays == 1 :
            dayreturned = (listofdays[numdays + indexofday] 
            + " next day")
        else:
            dayreturned = (listofdays[numdays + indexofday] + 
            f" ({numdays} days later)") 

        return  tothrs if tothrs != 0 else 12, dayreturned   

def add_time(start, duration, day=''):
  starting = start.split(" ")
  num = starting[0]
  form = starting[1]
  starttime = num.split(':')
  hours = starttime[0]
  mins = starttime[1]
  addedtime = duration.split(':')
  addedhrs = addedtime[0]
  addedmins = addedtime[1]
  tothrs = int(hours) + int(addedhrs)
  totmins = int(mins) + int(addedmins) 
  returnedstr = ''

  # incrementing hrs if totalmins exceed 60
  if(totmins > 60):
     tothrs += 1
     totmins = totmins - 60

  tothrs, returnedstr = get_days(int(tothrs), day.lower(), form)  

# getting the right form
  if form == 'AM':
    if tothrs >= 12:
      form = 'PM'
  elif form == 'PM':
    if tothrs >= 12:
      form = "AM"

# getting the right hrs
  if tothrs > 12:
       tothrs = tothrs - 12 

# getting the right mins format
  if totmins < 10:
      totmins = '0' + str(totmins)    

# final results
  if returnedstr != '': 
      print("{0}:{1} {2}, {3}".format(tothrs, totmins, form, returnedstr))
  else:
      print("{0}:{1}, {2}".format(tothrs, totmins, form, returnedstr))

Thanks a lot, @mukeshgurpude.

check out this see if it lacks something :upside_down_face: :upside_down_face:

#5

Dear @mootarek,
You’ve made your code much more readable and efficient.
For further optimization I would suggest use of modulo(%) operator and string formatting methods such as rjust, ljust.
I can’t assure you that this are best practices, they’ll reduce conditional statements in your code.
For example

You can do this with

tothrs += totmins//60
totmin %= 60

This can be achieved with str(totmins).rjust(2, '0')

I just want to say you that keep this attitude of making code efficient after finishing it. Most of the people leave the project as soon as they get the result without worrying about worse cases of input.
So keep it up :+1:

1 Like
#6

We can collaborate on a project if you have some time :wink:

#7

What type of project?

#8