Как правильно спроектировать эту функцию?

Question

[Kirgus]

Проект пишется на Python aiogram.
Есть функция show_cpanel, которая вызывается из эндпоинтов aiogram.
Есть 2 варианта реализации этой функции:

1. Сделать единую функцию, немного заморочившись с её параметрами. Тело функции будет немного больше.
   
2. Перегрузка функции с помощью декоратора overload. Тело функции будет немного меньше.
   

Вот эти 2 варианта:

async def show_cpanel(
    state: FSMContext,
    callback_query: Optional[CallbackQuery] = None,
    message: Optional[Message] = None,
) -> None:
    '''
    Send control panel to current user
    '''
    assert callback_query or message

    if callback_query:
        this_user = callback_query.from_user.id
        message = message or callback_query.message
    else:
        this_user = message.from_user.id
    profile_is_visible = await req.check_profile_visible(this_user)
    message_id = (await message.answer(
        '<b>' + _('Панель управления') + '</b>',
        parse_mode='HTML',
        reply_markup=kb.control_panel(profile_visible=profile_is_visible)
    )).message_id
    await state.update_data(cpanel_message_id=message_id)
    logger.info(f'Admin #{this_user} opened control panel')

или

@typing.overload
async def show_cpanel(
    state: FSMContext,
    callback_query: CallbackQuery
) -> None:
    '''
    Send control panel to current user
    '''

    this_user = callback_query.from_user.id
    profile_is_visible = await req.check_profile_visible(this_user)
    message_id = (await callback_query.message.answer(
        '<b>' + _('Панель управления') + '</b>',
        parse_mode='HTML',
        reply_markup=kb.control_panel(profile_visible=profile_is_visible)
    )).message_id
    await state.update_data(cpanel_message_id=message_id)
    logger.info(f'Admin #{this_user} opened control panel')


@typing.overload
async def show_cpanel(
    state: FSMContext,
    message: Message
) -> None:
    '''
    Send control panel to current user
    '''

    this_user = message.from_user.id
    profile_is_visible = await req.check_profile_visible(this_user)
    message_id = (await message.answer(
        '<b>' + _('Панель управления') + '</b>',
        parse_mode='HTML',
        reply_markup=kb.control_panel(profile_visible=profile_is_visible)
    )).message_id
    await state.update_data(cpanel_message_id=message_id)
    logger.info(f'Admin #{this_user} opened control panel')

Какой из вариантов будет правильнее с точки зрения чистоты кода и его дальнейшей поддержки?

Comments

[Wispik]

Ну сам посмотри, первый вариант по количеству кода меньше

[Алексей Уколов]

И без копипастинга.

[Kirgus]

Wispik, но вариант с перегрузкой более понятный и простой

Answer 1

[Everything_is_bad]

оба варианты неправильные (а второй вообще с тупой копипастой), а надо всего лишь вот это кусок

if callback_query:
        this_user = callback_query.from_user.id
        message = message or callback_query.message
    else:
        this_user = message.from_user.id

определять перед функцией, точнее выявлять message и передавать его в саму функцию, юзер же уже есть в этом message у CallbackQuery, да?

Comments

[Kirgus]

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

Answer 2

[Vindicar]

Ты неверно понимаешь, как работает overload. Это тупо подсказка для IDE - "эту функцию можно вызывать вот так, и она вернёт вот это, или же вот этак, и она вернёт вон то". Тело функции, завёрнутой в overload, игнорируется. А последнее объявление функции не должно иметь overload, и оно как раз и будет реализовывать все варианты. Так что итоговая реализация всё равно будет иметь if внутри. Да и в твоём случае overload никчему, так как другие-то параметры не отличаются.

Ты мог бы, конечно, упороться, и сделать functools.singledispatch - при условии, что ты message переставишь первым параметром. Но, имхо, в этом нет практической необходимости, всё это делается в разы проще.

async def show_cpanel(
    state: FSMContext,
    message_source: typing.Union[Message, CallbackQuery]  # значение одного из двух типов, но обязательное!
) -> None:
    '''
    Send control panel to current user
    '''
    this_user = message_source.from_user.id  # и Message, и CallbackQuery имеют from_user
    message = message if isinstance(message_source, Message) else message_source.message

    profile_is_visible = await req.check_profile_visible(this_user)
    message_id = (await message.answer(
        '<b>' + _('Панель управления') + '</b>',
        parse_mode='HTML',
        reply_markup=kb.control_panel(profile_visible=profile_is_visible)
    )).message_id
    await state.update_data(cpanel_message_id=message_id)
    logger.info(f'Admin #{this_user} opened control panel')

Вот и всё. Твой вариант

callback_query: Optional[CallbackQuery] = None,
    message: Optional[Message] = None,

плох тем, что из него совершенно не очевидно, что хотя бы один из параметров не должен быть None.

EDIT: Хотя я не вполне понял идею насчёт callback.message. Если я верно помню, этот атрибут хранит ссылку на сообщение от бота, содержащее кнопку, для которой был вызван callback. Поэтому его from_user по идее будет заведомо указывать на бота. Так что да, лучше последуй совету Everything_is_bad и переделай функцию так, чтобы она принимала пользователя и сообщение отдельными параметрами. А их значения определяй там, где ты функцию вызываешь.

Comments

[Everything_is_bad]

ну так может всё таки лучше вычисление message вообще из функции вынести? Я про то, зачем функция должна знать как его можно получить из всех возможных способов, она должна просто получать message, а выявления message должно делаться на уровне выше.

[Vindicar]

Everything_is_bad, это тоже вариант, да. Просто я не знаю, как у него распределены обязанности в коде, поэтому пока придерживаюсь более близкого к оригиналу варианта.

[Vindicar]

Everything_is_bad, погодь, я кое-что вспомнил...

[Kirgus]

Тело функции, завёрнутой в overload, игнорируется

И правда...
Досадно.

[Vindicar]

Kirgus, то, что ты пытался сделать, реализуется через functools.singledispatch. Но на практике обычно можно (и нужно) обойтись более простыми и очевидными способами.