3
\$\begingroup\$

I have this piece of code, which raises an exception if there are no numbers from 3 services. I really don't like how it looks at the moment.

no_numbers = False
if use_verizon_sms:
  if not Api.check_avalible_credits():
    no_numbers = True
elif use_verizon:
  if not buy_verizon_number():
    no_numbers = True
else:
  if not check_count_numbers():
    no_numbers = True

if no_numbers:
  raise Exception("THERE IS NO AVAILABLE NUMBERS")

A shorter version looks even more ugly to me and less clear:

if (use_verizon_sms and not Api.check_avalible_credits()) or \
  (use_verizon and not buy_verizon_number()) or \
  not check_count_numbers():
  raise Exception("THERE IS NO AVAILABLE NUMBERS")

Do you have any ideas how to rewrite it?

\$\endgroup\$
2
  • 3
    \$\begingroup\$ Welcome to CodeReview! It would really help if you could add a description of what this code is trying to achieve. Also you should add a more descriptive title, right now I have no idea what it is trying to say. \$\endgroup\$
    – Graipher
    Oct 13, 2016 at 18:28
  • 2
    \$\begingroup\$ That code alone looks just fine, but some context might change the situation. \$\endgroup\$
    – Oskar Skog
    Oct 13, 2016 at 18:31

2 Answers 2

6
\$\begingroup\$

Have a look whether you can extract the code into a function. It then may look like this.

def has_no_numbers():
  if use_verizon_sms:
    return Api.check_avalible_credits()

  if use_verizon:
    return buy_verizon_number()

  if check_count_numbers()
    return True

  raise Exception("THERE IS NO AVAILABLE NUMBERS")
\$\endgroup\$
4
  • 1
    \$\begingroup\$ I think the raise Exception('blah') should be replaced with return False. \$\endgroup\$
    – Oskar Skog
    Oct 13, 2016 at 19:10
  • 1
    \$\begingroup\$ @OskarSkog: When the code reaches the exception, it looks more like someone has forgotten to insert code for handling another "source of numbers", so the exception looks reasonable to me. \$\endgroup\$ Oct 13, 2016 at 19:26
  • \$\begingroup\$ Roland, that code behaves differently to the original. Did you mean to write, if has_no_numbers(): raise Exception (outside of the function)? \$\endgroup\$ Oct 14, 2016 at 10:04
  • \$\begingroup\$ This solution actually throws back 3 different types of results. Boolean, API's result and an Exception. Every time you'd use the block, you'd need to try/except trap it and try to figure out what result you got from it. Did I get a True? Did I get a False? Why is it True? Because of check_count_numbers? Did I need to buy_verizon numbers? What did my program just do? I have no idea what it did. \$\endgroup\$
    – C. Harley
    Mar 12, 2018 at 0:27
0
\$\begingroup\$

I would attempt something like this (I've written this snippet so you can read it through like english to understand the change program flow which is what the question is asking; not how I'd actually write my code).

def check_for_no_numbers():
    no_numbers = check_for_numbers()

    def check_for_numbers():
        return check_count_numbers()

    if not no_numbers:
        no_numbers = check_verizon(Api, use_verizon, use_verizon_sms)

    def check_verizon(Api, use_verizon, use_verizon_sms):
        if use_verizon_sms:
            result = Api.check_available_credits()
            if not result:
                return True

        if use_verizon:
            result = buy_verizon_number()
            if not result:
                return True

        return False

    return no_numbers


some_program_logic = check_for_no_numbers()

if not some_program_logic:
    raise Exception("THERE ARE NO AVAILABLE NUMBERS")
\$\endgroup\$

Your Answer

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

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