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\$
2
  • 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, 2018 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\$ Feb 16, 2018 at 9:38

2 Answers 2

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\$
6
  • \$\begingroup\$ if not description.startswith('-')? \$\endgroup\$ Feb 16, 2018 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, 2018 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\$ Feb 16, 2018 at 10:33
  • \$\begingroup\$ @FrerichRaabe You are correct, gen_response, or gen_reply, would be a better name. \$\endgroup\$
    – Peilonrayz
    Feb 16, 2018 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\$ Feb 19, 2018 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\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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