11
\$\begingroup\$

I have made a time format converter which converts 12 hour format to 24 hour format. My convert_to_24() takes a string in format of HH:MM:SS followed by AM or PM. It returns a converted string to 24 hours format without the AM/PM string.

Note: Let us assume that input will always have no space between them e.g. 07:05:45PM, not 07:05:45 PM

def convert_to_24(time):
    """str -> str
    converts 12 hours time format to 24 hours
    """
    return time[:-2] if time[-2:] == "AM" else str(int(time[:2]) + 12) + time[2:8]

Sample usage:

>>> convert_to_24("07:05:45PM")
'19:05:45'
>>> convert_to_24("10:47:00PM")
'22:47:00'
>>> convert_to_24("02:35:36AM")
'02:35:36'
>>> 

Can my function be made even better? By better I mean better in way of performance, design etc.

\$\endgroup\$
11
\$\begingroup\$

This function is not just cryptic — it's also wrong. I would expect convert_to_24('12:00:00AM') (midnight) to be '00:00:00', but it returns '12:00:00' (noon).

\$\endgroup\$
1
  • \$\begingroup\$ Oh my bad! :O Didn't notice. Will have to fix. \$\endgroup\$ Nov 24 '15 at 12:12
6
\$\begingroup\$

Potential improvements:

  • Increase clarity, this will need to use more than one line. And so you may dislike it.

  • Let us assume

    Always reminds me of quote/saying "assume -- makes an ass out of u and me".
    So I would say not to assume. (There is a easy fix anyway.)


Since this is tagged , and we're already assuming. We should decide what is the fastest way to do the if statement.

If we look at the check, you can input ...AM or ...PM. Since the last letter is always the same, there is no point in checking it is there?
But not only that not checking is faster by a factor of 10.

>>> timeit.timeit('"abcde"[:-2]')
0.2815848802857645
>>> timeit.timeit('"abcde"[-2]')
0.028008557233969

And so, if you use that it should be the fastest you can get it. (Unless datetime is faster.)

return time[:8] if time[-2] == "A" else str(int(time[:2]) + 12) + time[2:8]

Design improvements

Lets say you just scraped a dodgy web page and got ' 10:47:00 PM ' as the time. We would want your function to work. But it won't.

There is an easy way to fix this however! Just use str.strip().

>>> '  10:47:00   PM   '.strip()
'10:47:00   PM'

Now you have the beginning time, and the latter AM/PM in the correct places.

Now you want to return without the space. But I want to be able to put 1:30PM in the function, and it to work.
To do this you should use str.find(':') and str.rstrip():

>>> '  10:47:00   PM   '.find(':')
4
>>> '  10:47:00   PM   '.rstrip()
'  10:47:00   PM'

However using two str.strips seems counter intuitive. So if you assume that it won't have any of '\t\n\f's in it, then you can use str.replace.

>>> '  10:47:00   PM   '.replace(' ', '')
'10:47:00PM'

And now let's say I'm being really lazy today, I don't want to press shift, and only put in 1:30am. I would still expect it to work.
And again there is another simple fix.

>>> 'AM'.lower()
'am'

However we still haven't validated if the user actually entered AM or PM, and it's bad to assume they put in PM. Instead I would raise an error.
Just something simple will do:

raise ValueError("Didn't finish with AM or PM.")

I'd say it's good now that it works with ' 1:30:20.45 am ', but what if I just can't count today... and I just want to know what the hour is. I input 1pm, and I get some weird error. Not good.
This is as the program can't find a : and so returns -1. And so you should change to something more sensible.

index = time.find(':')
if index == -1:
    # Pick one depending on implementation
    index = -2
    index = None

And so, if you tie it all together.

def convert_to_24(time):
    """Converts 12 hours time format to 24 hours
    """
    time = time.replace(' ', '')
    time, half_day = time[:-2], time[-2:].lower()
    if half_day == 'am':
        return time
    elif half_day == 'pm':
        split = time.find(':')
        if split == -1:
            split = None
        return str(int(time[:split]) + 12) + time[split:]
    else:
        raise ValueError("Didn't finish with AM or PM.")

In short, assumptions are bad. Unless the assumption is the end user is going to put something unexpected in.

(Also this still errors if you put 'abc:de:fAM'.)

\$\endgroup\$
1
  • \$\begingroup\$ @holroy I have to admit, I've read clocks wrong my whole life... \$\endgroup\$
    – Peilonrayz
    Nov 23 '15 at 21:46
4
\$\begingroup\$

There are 2 approaches:

Using the time library:

import time
t = '12:59 AM'
strt = time.strptime(t, '%I:%M %p')
print(time.strftime('%H:%M', strt))

Pro: Simple / Cons: Requires very strict formating of the input string.

Use regex:

If the input string is not that strict, and you end up with a regex. Just go the regex & conditions way:

t = " 12:59 AM "
import re
r = re.compile('''
  ^\s*
  ([01]?\d)  # the time in hours
  (\W.*)     # everything in between (to reinsert it at the print)
  ([ap][m])  # the am/pm 
  \s*$
  ''', re.IGNORECASE + re.VERBOSE)
m = r.match(t)

if m:
    print("Match : " + str(m.groups()))

    hh = int(m.group(1))
    ampm = m.group(3)

    hh= 0 if hh==12 else hh                   # Fix the 12AM/12PM oddity
    hh= hh+12 if "PM" in ampm.upper() else hh # Add 12 if PM
    print('{0:02}{1:}'.format(hh, m.group(2)))
else:
    print("Provided time '" + t + "' does match")
\$\endgroup\$
3
\$\begingroup\$

Joe Wallis gave some very good ideas for improvement, but even if you didn't at all change how it worked, you have a very long messy ternary. There's so much packed in there it's confusing. Splitting it into if else will read as a lot clearer.

def convert_to_24(time):
    """str -> str
    converts 12 hours time format to 24 hours
    """

    if time[-2:] == "AM":
        return time[:-2]
    else:
        return str(int(time[:2]) + 12) + time[2:8]

Now it's a lot easier to see what ending with "AM" means and how you're handling "PM".

\$\endgroup\$
1
\$\begingroup\$

The problem with the above code is for 12:00:00 AM and 12:00:00 PM it gives the answers as 12:00:00 and 24:00:00 respectively as it checks whether it is AM and returns the time as it is without "AM" and adds 12 in hour part if not.But the code below checks whether the hour is 12(AM or PM) and gives the answer accordingly.

Eg:"12:ab:cd AM" is converted to "00:ab:cd" and "12:ab:cd PM" is converted to "12:ab:cd".

The rest is same as the above code.

def convert_to_24(time):
    if time[-2:] == "AM" and time[:2] == "12":
        return "00"+time[2:-2]
    elif time[-2:] == "AM":
        return time[:-2]
    elif time[-2:] == "PM" and time[:2] == "12":
        return time[:-2]
    else:
        return str(int(time[:2]) + 12) + time[2:8]
\$\endgroup\$
0
0
\$\begingroup\$

I think it would be more efficient if we use python inbuilt datetime module. If there is no space, we add space before converting

import datetime
dt = "01:05:45PM" 
dt=dt.replace("PM"," PM") # adds space if there is PM
dt=dt.replace("AM"," AM") # adds space if there is AM
time_object = datetime.datetime.strptime(dt, '%I:%M:%S %p').time()
print(time_object)

Sample output :

13:05:45
\$\endgroup\$

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.