Time Format Conversion Utility

Question

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).

    Args:
        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.'.

    Note:
    - 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
      strings.
    """
    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 '
    else:
        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__':
    _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.

Answer 1

#!/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.

---

enum

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.

---

exceptions

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.

---

representation

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.

EDIT

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.

Answer 2

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'

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

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

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

    # etc.


if __name__ == '__main__':
    test_all()