1
\$\begingroup\$

I'm writing a bot, it turns out pretty big (1000 lines). In code, I often use the "show profile" function. I noticed that I greatly complicate it by adding functionality with kwargs. I am a newbie, I don’t know if I should create a class instead of a function, break it into different functions or continue in the same manner.

Now I need to add 2 features:

  1. "show profile when creating for confirmation"
  2. "Keyboard selection", if you want to show a group of profiles (the most important)

Basically, these problems arise because I make a bot using a module and in the conditions of a strict API. If you have some remarks related to security, performance, etc - please, tell me them.

Sum up: I have three functions:

  1. get_keyboard
  2. show_profile
  3. show_group_of_profiles

show_group_of_profiles must call show_profile, and show_profile must call get_keyboard. But show_group_of_profiles must call get_keyboard (via show_profile) with a specific argument, that in reality hardcoded in show_profile. I can pass this argument via kwargs, but it is a bad approach I think

def show_profile(user_id, profile_data=None, photo=None):

""" Profile it is just a message object with photo, text (caption) and (inline) keyboard (markup). 
    Photo=None because While a recreating a new profile, old photos will be taken from DB, 
    but you can't remove them until a user will approve a new profile (first missing feature). 
    db_execute - manually created function, the short version of cursor.execute (pymysql)"""

    if not profile_data:
        profile_data = db_execute('SELECT goal, age, gender, country, city, comment FROM users WHERE user_id = %s',
                                  user_id, fetchone=True, get_tuple=True)
    else:
        profile_data = list(profile_data.values())

    if not photo:
        photo = db_execute('SELECT photo FROM photos WHERE user_id = %s and id = (SELECT MIN(id) FROM photos)',
                           user_id, fetchone=True)
        if not photo:
            photo = DEFAULT_PHOTO[0]

    username = bot.get_chat(user_id).username  # Get first and last name of user
    caption = f'Имя - <a href="tg://user?id={user_id}">{username}</a>.\n'  # add link to user inside a message
    profile_data = {'Goal': profile_data[0], 'Agr': profile_data[1], 'Gender': profile_data[2],
                    'Country': profile_data[3], 'City': profile_data[4], 'Comment': profile_data[5]}

    # Remove None data from profile
    for key, value in dict(profile_data).items():  # Dict() to avoid 'dictionary changed size during iteration' error
        if value is None:
            del profile_data[key]
        else:
            caption += f'{key} - {value}\n'

    message = bot.send_photo(user_id, photo, caption=caption, parse_mode=ParseMode.HTML,
                             reply_markup=get_scrolling_profile_keyboard(user_id))
    return message  # To allow edits


def get_scrolling_photos_keyboard(shower_id):  
    return InlineKeyboardMarkup([[
        InlineKeyboardButton('<', callback_data=f'back_photo {shower_id}'),
        InlineKeyboardButton('>', callback_data=f'next_photo {shower_id}')]])


def get_scrolling_profiles_keyboard(shower_id):

""" Add more buttons for scrolling, group of photos and group of profiles 
(the profile is still just a message object with photo, text, and buttons)
(The second missing feature)"""

    keyboard_obj = get_scrolling_photos_keyboard(shower_id)
    buttons = keyboard_obj.inline_keyboard  # Get list from object
    buttons.append(InlineKeyboardButton('Back', callback_data=f'back_profile'))
    buttons.append(InlineKeyboardButton('Next', callback_data=f'next_profile'))
    return InlineKeyboardMarkup(buttons)
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Welcome to CodeReview@SE. Please clarify, in your question, whether you are seeking for a refactoring including extending functionality, or in preparation of an extension. Have (another) look at How do I ask a Good Question? \$\endgroup\$
    – greybeard
    Apr 18 '20 at 4:46
  • \$\begingroup\$ Thank you for the comment. I don't familiar with these terms, because I will googling before answering you \$\endgroup\$ Apr 18 '20 at 5:02
  • \$\begingroup\$ I don't get what you mean, cuz I will just answer you what I want: I have three functions 1. get_keyboard 2. show_profile 3. show group of profiles So, function 3 must use function 2, and function 2 must use function 1. But function 3 must call function 1 (via function 2) with a specific argument, that in reality hardcoded in function 2. I can pass this argument via kwargs, but it is a bad approach I think \$\endgroup\$ Apr 18 '20 at 5:46
  • \$\begingroup\$ I think the question is whether those 2 'new functions' you talk about are already implemented in the current code or not. The descriptions you gave of them do not completely correlate with the actual functions provided. Please clarify. \$\endgroup\$
    – Mast
    Apr 18 '20 at 15:23
  • 1
    \$\begingroup\$ I have voted to close this as the code is not working as you intend. The first feature has not been created yet. To remove my close vote please either 1. Remove any reference to features that are not implemented yet and just be happy with a review of the code as it is now. 2. Add the required features and edit the question or post a new question with the fully working code. \$\endgroup\$
    – Peilonrayz
    Apr 19 '20 at 1:56
0
\$\begingroup\$

I can't comment much on the questions you asked about the functions that haven't been implemented yet, other than to say that passing an argument down through multiple function calls is perfectly fine. If your code cleanly declares what arguments it expects and what each argument does it shouldn't be a problem. I did my best to clean up the code you provided, mostly just fixing PEP8 style stuff and trying my best to fix the typing (although it's hard without having any of your dependencies to check against, I tried to tidy up stuff like having profile_data go from dict to list back to dict). I also replaced the janky in-place dictionary modification with a comprehension.

from typing import Dict, Optional


def show_profile(
    user_id: str,
    profile_data: Optional[Dict[str, str]] = None,
    photo: Optional[str] = None
) -> Message:  # fixme: what's the actual type of this?
    """
    A profile is a message object with a photo, text (caption), and markup.
    If no profile data is specified it will be loaded from the db.
    If no photo is specified, it will be loaded from the db, 
    or DEFAULT_PHOTO will be used.
    """
    # db_execute - manually created function,
    # the short version of cursor.execute (pymysql)"""
    if profile_data is None:
        goal, age, gender, country, city, comment = db_execute(
            'SELECT goal, age, gender, country, city, comment '
            'FROM users '
            'WHERE user_id = %s',
            user_id, fetchone=True, get_tuple=True
        )
        profile_data = {
            'Goal': goal,
            'Age': age,
            'Gender': gender
            'Country': country,
            'City': city,
            'Comment': comment
        }

    if photo is None:
        photo = db_execute(
            'SELECT photo '
            'FROM photos '
            'WHERE user_id = %s and id = (SELECT MIN(id) FROM photos)',
            user_id, fetchone=True
        ) or DEFAULT_PHOTO[0]

    # Get first and last name of the user.
    username = bot.get_chat(user_id).username

    # add link to user inside a message
    caption = f'Имя - <a href="tg://user?id={user_id}">{username}</a>.\n'

    # Remove None data from profile
    profile_data = {k: v for k, v in profile_data.items() if v is not None}
    # Add it to the caption
    for k, v in profile_data.items():
        caption += f'{key} - {value}\n'

    return bot.send_photo(
        user_id,
        photo,
        caption=caption,
        parse_mode=ParseMode.HTML,
        reply_markup=get_scrolling_profile_keyboard(user_id)
    )


def get_scrolling_photos_keyboard(shower_id: str) -> InlineKeyboardMarkup:
    return InlineKeyboardMarkup([[
        InlineKeyboardButton('<', callback_data=f'back_photo {shower_id}'),
        InlineKeyboardButton('>', callback_data=f'next_photo {shower_id}')]])


def get_scrolling_profiles_keyboard(shower_id: str) -> InlineKeyboardMarkup:
    """
    Add more buttons for scrolling, group of photos and group of profiles
    (the profile is still just a message object with photo, text, and buttons)
    (The second missing feature)
    """
    keyboard_obj = get_scrolling_photos_keyboard(shower_id)
    buttons = keyboard_obj.inline_keyboard  # Get list from object
    buttons.append(InlineKeyboardButton('Back', callback_data=f'back_profile'))
    buttons.append(InlineKeyboardButton('Next', callback_data=f'next_profile'))
    return InlineKeyboardMarkup(buttons)
\$\endgroup\$

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