Amplicon
Исходный код:
Исходный код:
Начнём с того, что функция arrival_time()
сейчас выполняет слишком много разных действий. Она:
Такие функции называют божественными. Они работают, но проблема в том, что их неудобно отлаживать. Проблемы могут возникнуть на разных этапах, и придётся предусмотреть целую кучу тестовых сценариев, чтобы покрыть все возможные исходы. Да и в целом работать с функцией, которая единолично выполняет сразу несколько разных действий, не слишком удобно.
Поэтому вынесем в отдельную функцию ту часть логики, которая отвечает за превращение строки вида "14:30"
в кортеж вида (14, 30)
. Пока что максимально сохраняем исходный вид кода, внося лишь самые необходимые изменения.
Запустите код, чтобы убедиться, что он по-прежнему работает и проходит тесты.
Теперь посмотрим повнимательнее на функцию 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 переменных не получится. Запустите код ниже, чтобы в этом убедиться.
То есть вроде бы предусмотрели блок try/except
, а в итоге всё равно получаем то же самое исключение, которое хотели поймать, только в другом месте.
Правильный подход выглядит так: функция выбрасывает исключение, оно «вываливается» из функции в точку вызова и там его ловят и обрабатывают с помощью try/except
. Например:
Минуты - это число от 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
. Иначе мы получаем вот такую картину:
Также стоит учитывать, что в текущем виде проверка пропускает отрицательные числа. Давайте поправим этот нюанс.
Встроим эту функцию в общий код:
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
для консистентности :)