8
\$\begingroup\$

So far, I've created a couple of programs, and the issue that comes up is how ugly and non-scalable they look.

For example, currently I'm working on a telegram bot. Before replying to a message I need to check that the message follows a couple of rules. In order to minimize the number of if/elif statements. I've even created a couple of functions that return True or False if my condition is met, but the code still looks very ugly:

# only if messages are not empty and description has a hyphen
if channel and (description and description[0] == '-'):

    if is_channel(channel) and is_length(description):

        if channel_is_size(channel):
            bot.reply_to(message, replies.success_add.format(channel))
            write_results(' '.join(message_words[1:]))
        else:
            bot.reply_to(message, replies.small_chan.format(min_channel_size))

    else:
        bot.reply_to(message, replies.enter_chan)

elif not description and channel:
    bot.reply_to(message, replies.enter_desc_error)
else:
     bot.reply_to(message, replies.enter_addmessage_error) # if empty

As you can see, these are the conditions for which the user message will be accepted into the database (it's a text file), and this can be seen right under the triple nested if statements. I've tried to make this code look nicer many times over, but now it seems very messy and if I would want to add more conditions, which I am planning to do, these nested statements will grow out of proportion.

\$\endgroup\$
  • 5
    \$\begingroup\$ Welcome to CodeReview, could you maybe add some example data of a message? To me it is not very clear what the input/output should be, nor what those functions do is_length etc... \$\endgroup\$ – Ludisposed Feb 16 '18 at 8:58
  • 1
    \$\begingroup\$ What is description, is it always a str (albeit possibly empty)? or can it be something else like None? \$\endgroup\$ – Mathias Ettinger Feb 16 '18 at 9:38
12
\$\begingroup\$

If you want to get rid of your nested code then you should make a function and use guard statements.

If we simplify your code a bit, we can have:

if C and D and D[0] == '-':
    return a  # Ignoring other code, for simplicity
elif C and not D:
    return b
else:
    return c

Instead you can have:

if C:
    if not D:
        return b
    elif D[0] == '-':
        return a
return c

If you use the guard clause then you get:

if not C:
    return c
if not D:
    return b
if D[0] != '-':
    return c
return a

Using the same logic, you can expand all your ifs to guard clauses. And the rest is easier than the above to change. Which can leave you with:

def gen_response(channel, description, replies, message_words):
    if not channel:
        return replies.enter_addmessage_error

    if not description:
        return replies.enter_desc_error

    if description[0] != '-':
        return replies.enter_addmessage_error

    if not (is_channel(channel) and is_length(description)):
        return replies.enter_chan

    if not channel_is_size(channel):
        return replies.small_chan.format(min_channel_size)

    write_results(' '.join(message_words[1:]))
    return replies.success_add.format(channel)


bot.reply_to(message, gen_response(channel, description, replies, message_words))
\$\endgroup\$
  • \$\begingroup\$ if not description.startswith('-')? \$\endgroup\$ – Mathias Ettinger Feb 16 '18 at 9:42
  • \$\begingroup\$ @MathiasEttinger You can do that, but that's a simpler way of using if d and d[0] == '-', and since we've already done if d I didn't feel the need to change it. \$\endgroup\$ – Peilonrayz Feb 16 '18 at 9:44
  • \$\begingroup\$ It seems odd to have the first argument to reply_to called message and the second argument being a call to gen_message. How about renaming the latter to gen_reply (which seems fitting given that the various replies are in the replies namespace, and the method is called reply_to? \$\endgroup\$ – Frerich Raabe Feb 16 '18 at 10:33
  • \$\begingroup\$ @FrerichRaabe You are correct, gen_response, or gen_reply, would be a better name. \$\endgroup\$ – Peilonrayz Feb 16 '18 at 10:37
  • \$\begingroup\$ in my case, 'replies' is just a python file containing all the messages needed for the program. I'm not sure you need to add it as an argument for the function gen_response \$\endgroup\$ – Komron Aripov Feb 19 '18 at 16:57
4
\$\begingroup\$

Interesting question. We can solve the problem (at least partially) by changing the order of the if statements. I'm assuming you'll want to add more conditions in the first clause, which means we can 'fall through' early:

if not description and not channel:
    # Empty message
    bot.reply_to(message, replies.enter_addmessage_error)

elif not description:
    # Non-empty message, empty description
    bot.reply_to(message, replies.enter_desc_error)

elif description[0] == "-":
    # Non-empty message, non-empty description starting with a hyphen
    if is_channel(channel) and is_length(description):
        if channel_is_size(channel):
            bot.reply_to(message, replies.success_add.format(channel))
            write_results(' '.join(message_words[1:]))
        else:
            # `not channel_is_size(channel)`
            bot.reply_to(message, replies.small_chan.format(min_channel_size))
    else:
        # `not is_channel(channel) and not is_length(description)`
        bot.reply_to(message, replies.enter_chan)

This makes the code look slightly cleaner and allows for other early-return clauses.

It's hard to tell what the context of this piece of code is, but another solution may be to call helper functions, e.g.

def foo(channel, description):
    # No idea what to call this, frankly

def bar():
    # idem dito
    ...
    elif description[0] == "-":
        foo(channel, description)
    ...

This will certainly help with scaling later on.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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