The code defines a Python script for converting time between 12-hour and 24-hour formats.

Review Goals:

  1. Is the code idiomatic? (Am I using any outdated practices or functions?)
  2. Is there a simpler way to achieve the same thing?
  3. Are the tests well thought of?
  4. Does any part of my code require a comment?
  5. How can I improve the code?

The code was formatted with autopep8.

#!/usr/bin/env python3

def _convert_to_24_hr(hr: int, mins: int, time_ind: str) -> str:
    if time_ind.upper() == 'PM':
        hr = hr + 12 if hr != 12 else 0
    return f'{hr}:{mins:0>2} {time_ind.upper()}'

def _convert_to_12_hr(hr: int, mins: int) -> str:
    time_ind = 'PM' if hr >= 12 else 'AM'
    hr = 12 if hr == 0 else (hr - 12 if hr > 12 else hr)
    return f'{hr}:{mins:0>2} {time_ind.upper()}'

def time_conversion(hr: int, mins: int, fmt: str, time_ind: str = None) -> str:
    """Converts a time from one format to another (either 12-hour or 24-hour).

        hr: The hour part of the time.
        mins: The minute part of the time.
        format: A string representing the desired time format ('12-hour'
                or '24-hour').
        time_ind: An optional time indicator ('AM' or 'PM') used in 12-hour
                  format conversion.

    Returns: The converted time as a string in the specified format 
             on success. Else, it returns 'Invalid arguments.'.

    - If '24-hour' format is selected, the time_ind argument must be provided.
    - The function handles both upper and lower case format and time indicator
    conditions_24 = [1 <= hr <= 12,
                     0 <= mins <= 59,
                     time_ind and time_ind.upper() in ['AM', 'PM']]

    conditions_12 = [0 <= hr <= 23,
                     0 <= mins <= 59]

    if fmt.lower() == '24-hour':
        if all(conditions_24):
            # Valid 12-hour format
            return _convert_to_24_hr(hr, mins, time_ind)
        return 'Invalid arguments.'

    if fmt.lower() == '12-hour':
        if all(conditions_12):
            # Valid 12-hr format
            return _convert_to_12_hr(hr, mins)
    return 'Invalid arguments.'

def _test(got: str, expected: str) -> None:
    if got == expected:
        prefix = ' OK '
        prefix = ' X '
    print(f'{prefix.ljust(4)} | got: {got.ljust(20)} | '
          f'expected: {expected.ljust(20)}')

def _main():
    _test(time_conversion(15, 30, '12-hour'), '3:30 PM')
    _test(time_conversion(3, 30, '24-hour', 'PM'), '15:30 PM')
    _test(time_conversion(13, 30, '24-hour', 'AM'), 'Invalid arguments.')
    _test(time_conversion(25, 20, '12-hour'), 'Invalid arguments.')
    _test(time_conversion(13, 5, '12-hour'), '1:05 PM')
    _test(time_conversion(13, 5, '24-hour'), 'Invalid arguments.')
    _test(time_conversion(10, 5, '24-hour', 'am'), '10:05 AM')
    _test(time_conversion(10, 5, '24-hour', 'PM'), '22:05 PM')
    _test(time_conversion(17, 5, '12-hour'), '5:05 PM')
    _test(time_conversion(25, 32, '12-hour'), 'Invalid arguments.')
    _test(time_conversion(0, 23, '12-hour'), '12:23 AM')
    _test(time_conversion(12, 0, '12-hour'), '12:00 PM')
    _test(time_conversion(-4, -3, '13-hour'), 'Invalid arguments.')
    _test(time_conversion(1003, 29, '39 hour', None), 'Invalid arguments.')

if __name__ == '__main__':

P.S: I would like to mention that this is my first Python script, or at least the first one I am posting for review. Feel free to expatiate.

  • \$\begingroup\$ A bug: _convert_to_12_hh erroneously converts midnight to noon. time_ind = 'PM' if hh >= 12 else 'AM' ==> time_ind = 'PM' if hh >= 12 and hh != 0 else 'AM'. It is fixed now. \$\endgroup\$
    – Harith
    Sep 29 at 19:42
  • \$\begingroup\$ 15:30 PM is kind of overkill, isn't it? With the 24-hour system, there is no need for the indicator \$\endgroup\$
    – TomG
    Sep 30 at 5:16
  • 1
    \$\begingroup\$ @TomG - I would see 15:30 PM, or any other 24 hour format time with am/pm following (e.g. 08:30 AM) as an error. Either 15:30 or 3:30 PM (in fact for me 15.30 or 3.30pm). \$\endgroup\$ Sep 30 at 21:16

2 Answers 2

#!/usr/bin/env python3

Nice shebang! Very portable.

Should comments be added? No, everything was quite clear.

plural agreement

def _convert_to_24_hr(hr: int, mins: int, time_ind: str) -> str:

Thank you for the hinting.

I suppose you wanted to avoid the identifier min, fair enough. But being out of step with singular "hour" is distracting. Consider using {hrs, mins}, or mn, or min_, or perhaps {hh, mm}.

mod math

The hr + 12 if hr != 12 else 0 expression is nice enough, but it kind of screams "modulo 24" to me. Which then would admit of hr += ... And down in _convert_to_12_hr things only get worse.

Single-digit formatting with f'{hr}: seems odd for 24-hour time. I would expect to see leading zeros, e.g. "00:00" for midnight.


Thank you for the very nice docstring in time_conversion, I found it helpful.

For one or both of the last two args, consider demanding that an Enum be passed in. Then we wouldn't need .lower() translations.


The docs explain: "Else, it returns 'Invalid arguments.'".

Consider a raise of ValueError instead. This would affect your non-standard unit tests, I feel in a good way, as you could then code some of them using a with self.assertRaises context handler.

Prefer to write a test suite in the usual way. Or use pytest if you'd rather.


Dealing with midnight / zero-hour is a rough edge in this codebase.

Consider turning hours and minutes immediately into a datetime and then rely on .strftime() for output. Doing that would also enable expressions like adding a 12-hour timedelta, but at that point you'd no longer have a need for adding twelve hours.


Thank you @Reinderien, quite right, I intended a time object. I'm afraid my code seldom talks about HH:MM outside the context of "some particular instant" that happened N seconds after 1970.

comments lie!

        if all(conditions_24):
            # Valid 12-hour format

Looks like a copy-pasta error.

This codebase achieves its design goals.

I would be willing to delegate or accept maintenance tasks on it.


Follow J_H's advice.

It's important that you separate this function into two, with a sane intermediate in-memory object (a datetime.time - not a datetime.datetime).

'22:05 PM' is not a useful format and probably shouldn't be supported.

_test is not useful and should be replaced with a simple assert.

This boils down to:

#!/usr/bin/env python3

from datetime import time
from typing import Literal

def make_time(hour: int, minute: int, meridiem: None | Literal['AM', 'PM'] = None) -> time:
    if meridiem in {None, 'AM'}:
        return time(hour, minute)
    if meridiem != 'PM':
        raise ValueError('Invalid meridiem')
    if hour > 12:
        raise ValueError('An hour should not be both greater than 12 and PM')
    return time(hour + 12, minute)

def fmt_time(time_of_day: time) -> str:
    return time_of_day.strftime('%I:%M %p').lstrip('0')

def test_all():

    assert (t := make_time(15, 30)) == time(15, 30)
    assert fmt_time(t) == '3:30 PM'

    assert (t := make_time(3, 30, 'PM')) == time(15, 30)
    assert fmt_time(t) == '3:30 PM'

        make_time(13, 30, 'PM')
        raise AssertionError()
    except ValueError:

        make_time(25, 20)
        raise AssertionError()
    except ValueError:

    assert (t := make_time(13, 5)) == time(13, 5)
    assert fmt_time(t) == '1:05 PM'

    # etc.

if __name__ == '__main__':

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.