Amplicon

Исходный код:

[brython-snippet]def arrival_time(depart, duration): try: h_dep, m_dep, h_dur, m_dur = map(int, depart.split(':') + duration.split(':')) except Exception: return 'ValueError(String values are not permitted)' try: if not all([h_dep < 24, m_dep or m_dur < 60]): raise ValueError("Invalid time input") except Exception as error: return error hh = (h_dep + h_dur + (m_dep + m_dur) // 60) % 24 mm = (m_dep + m_dur) % 60 return f"{hh :02d}:{mm :02d}" test_cases = [ ('14:30', '00:35', '15:05'), ('23:50', '01:00', '00:50'), ('00:10', '25:40', '01:50'), ] for case in test_cases: *args, expected = case actual = arrival_time(*args) message = f'{args}: ожидалось {expected}, получено {actual}' assert actual == expected, message print('Правильно!')[/brython-snippet]

Божественная функция

Начнём с того, что функция arrival_time() сейчас выполняет слишком много разных действий. Она:

разбивает строку, в которой содержится время, на части;
затем проверяет, что каждая часть соответствует определённым критериям;
затем рассчитывает количество часов/минут;
и в конце собирает из всего этого строку в нужном формате.

Такие функции называют божественными. Они работают, но проблема в том, что их неудобно отлаживать. Проблемы могут возникнуть на разных этапах, и придётся предусмотреть целую кучу тестовых сценариев, чтобы покрыть все возможные исходы. Да и в целом работать с функцией, которая единолично выполняет сразу несколько разных действий, не слишком удобно.

Поэтому вынесем в отдельную функцию ту часть логики, которая отвечает за превращение строки вида "14:30" в кортеж вида (14, 30). Пока что максимально сохраняем исходный вид кода, внося лишь самые необходимые изменения.

[brython-snippet]def parse_time(time): try: return tuple(map(int, time.split(':'))) except Exception: return 'ValueError(String values are not permitted)' def arrival_time(depart, duration): h_dep, m_dep = parse_time(depart) h_dur, m_dur = parse_time(duration) try: if not all([h_dep < 24, m_dep or m_dur < 60]): raise ValueError("Invalid time input") except Exception as error: return error hh = (h_dep + h_dur + (m_dep + m_dur) // 60) % 24 mm = (m_dep + m_dur) % 60 return f"{hh :02d}:{mm :02d}" test_cases = [ ('14:30', '00:35', '15:05'), ('23:50', '01:00', '00:50'), ('00:10', '25:40', '01:50'), ] for case in test_cases: *args, expected = case actual = arrival_time(*args) message = f'{args}: ожидалось {expected}, получено {actual}' assert actual == expected, message print('Правильно!')[/brython-snippet]

Запустите код, чтобы убедиться, что он по-прежнему работает и проходит тесты.

Слишком широкий блок except

Теперь посмотрим повнимательнее на функцию parse_time(time). Предполагается, что аргумент time будет выглядеть так: '14:30'. Но если в функцию передать неверные данные, например, 'abcde', то при попытке привести такую строку к int возникнет исключение ValueError.

Автор кода предусмотрел это и добавил конструкцию try/except. Но проблема в том, что блок except слишком «широкий», то есть в нём ловится не только ValueError, но и вообще все исключения, унаследованные от Exception. То есть если внутри блока try возникнет IndexError, TypeError, KeyError или что-то ещё, то все эти ситуации будут обработаны так:

return 'ValueError(String values are not permitted)'

Поэтому делаем блок except более узконаправленным:

def parse_time(time):
    try:
        return tuple(map(int, time.split(':')))
    except ValueError:
        return 'ValueError(String values are not permitted)'

Обработка исключений

Двигаемся дальше: сейчас мы ловим исключение ValueError, чтобы вернуть строку с текстом 'ValueError(...)'. В итоге функция может вернуть как кортеж из 2 чисел (часы/минуты), так и строку с произвольным текстом. Это значительно усложняет работу с функцией. Представьте, что вы хотите использовать её так:

time = input('Введите время: ')
hours, minutes = parse_time(time)

А пользователь взял и ввёл какое-нибудь некорректное время, например, 'abcde'. И что в итоге? А в итоге всё равно возникнет ValueError, потому что распаковать строку 'ValueError(String values are not permitted)' в 2 переменных не получится. Запустите код ниже, чтобы в этом убедиться.

[brython-snippet]def parse_time(time): try: return tuple(map(int, time.split(':'))) except ValueError: return 'ValueError(String values are not permitted)' time = input('Введите время: ') hours, minutes = parse_time(time)[/brython-snippet]

То есть вроде бы предусмотрели блок try/except, а в итоге всё равно получаем то же самое исключение, которое хотели поймать, только в другом месте.

Правильный подход выглядит так: функция выбрасывает исключение, оно «вываливается» из функции в точку вызова и там его ловят и обрабатывают с помощью try/except. Например:

[brython-snippet]def parse_time(time): return tuple(map(int, time.split(':'))) while True: time = input('Введите время: ') try: hours, minutes = parse_time(time) break except ValueError: print('Нормальное время введите, во имя святых печенюшек!')[/brython-snippet]

Проверяем часы и минуты

Минуты - это число от 0 до 59, а часы могут быть либо просто неотрицательным числом, либо числом от 0 до 23, в зависимости от ситуации.

Сейчас соответствующая проверка выполняется вот так:

if not all([h_dep < 24, m_dep or m_dur < 60]):

Встроенная функция all() очень удобна, когда нужно проверить, что условие выполняется для всех элементов итерируемого объекта. Но в данном случае итерируемый объект специально собирается из 2 отдельных условий, что несколько нецелесообразно. Можно просто обойтись логическим оператором and:

if not (h_dep < 24 and (m_dep or m_dur < 60)):

Ещё есть ощущение, что под m_dep or m_dur < 60 всё-таки имелось в виду m_dep < 60 and m_dur < 60. Иначе мы получаем вот такую картину:

[brython-snippet]h_dep = 23 m_dep = 99999 m_dur = 59 print(m_dep or m_dur < 60) print(h_dep < 24 and (m_dep or m_dur < 60)) print(not (h_dep < 24 and (m_dep or m_dur < 60)))[/brython-snippet]

Также стоит учитывать, что в текущем виде проверка пропускает отрицательные числа. Давайте поправим этот нюанс.

[brython-snippet]INFINITY = float('inf') def check_bounds(x, a=-INFINITY, b=INFINITY): if not (a <= x <= b): raise ValueError(f'{x} выходит за границы {a}..{b}') try: check_bounds(20, 0, 10) except ValueError as e: print(e) try: check_bounds(20, b=0) except ValueError as e: print(e) try: check_bounds(20, a=30) except ValueError as e: print(e)[/brython-snippet]

Встроим эту функцию в общий код:

INFINITY = float('inf')


def check_bounds(x, a=-INFINITY, b=INFINITY):
    if not (a <= x <= b):
        raise ValueError(f'{x} выходит за границы {a}..{b}')


def parse_time(time, within_day=False):
    hours, minutes = map(int, time.split(':'))

    check_bounds(minutes, 0, 59)
    if within_day:
        check_bounds(hours, 0, 23)
    else:
        check_bounds(hours, 0)

    return hours, minutes


def arrival_time(depart, duration):
    h_dep, m_dep = parse_time(depart, within_day=True)
    h_dur, m_dur = parse_time(duration)

    hh = (h_dep + h_dur + (m_dep + m_dur) // 60) % 24
    mm = (m_dep + m_dur) % 60

    return f'{hh :02d}:{mm :02d}'

Названия объектов

Названия объектов - это важнейшая составляющая чистого кода. Можно сколько угодно заниматься разделением логики на маленькие функции, но если имена переменных, функций и других объектов не передают их суть, то общая понятность кода тут же снижается до нуля. Сейчас всё далеко не так плохо, чтобы понятность кода стала нулевой, но определённые улучшения внести можно.

arrival_time() - это «время прибытия». Это существительное, описывающее предмет. Но оказывается, что таким именем названа функция, то есть действие. Можно использовать вариант calc_arrival_time(), чтобы более чётко показать, что эта функция рассчитывает время прибытия. Многие любят использовать get() в качестве универсального глагола для любых ситуаций, но лично мне не очень нравится эта практика. Есть много хороших слов: calculate, extract, parse и так далее. Они лучше передают суть, чем абстрактное get.

durat - в принципе, ясно, что это сокращение от duration. Но стоит ли экономить 3 символа, немного сокращая слово? Лучше просто использовать полное слово и не усложнять жизнь себе и коллегам.

hh, mm - чуть выше используются названия h_dep, h_dur. Что-нибудь вроде h_arrival более наглядно передало бы суть. Ну или хотя бы h_arr и m_arr для консистентности :)

Итоговый вариант кода

[brython-snippet]INFINITY = float('inf') def check_bounds(x, a=-INFINITY, b=INFINITY): if not (a <= x <= b): raise ValueError(f'{x} выходит за границы {a}..{b}') def parse_time(time, within_day=False): hours, minutes = map(int, time.split(':')) check_bounds(minutes, 0, 59) if within_day: check_bounds(hours, 0, 23) else: check_bounds(hours, 0) return hours, minutes def calc_arrival_time(departure, duration): h_dep, m_dep = parse_time(departure, within_day=True) h_dur, m_dur = parse_time(duration) h_arr = (h_dep + h_dur + (m_dep + m_dur) // 60) % 24 m_arr = (m_dep + m_dur) % 60 return f'{h_arr:02d}:{m_arr:02d}' test_cases = [ ('14:30', '00:35', '15:05'), ('23:50', '01:00', '00:50'), ('00:10', '25:40', '01:50'), ] for case in test_cases: *args, expected = case actual = calc_arrival_time(*args) message = f'{args}: ожидалось {expected}, получено {actual}' assert actual == expected, message print('Правильно!')[/brython-snippet]

Разбор решений к задаче «Мы уже приехали?»