2

How can I raise an exception here to catch nonpositive inputs? Right now nothing gets printed if I input a negative number

"""
Generate fibonacci sequence to the nth digit
"""
def fib(n):
    try:
        if n <= 0:
            raise Exception
        prev = 0
        curr = 1
        for terms in range(0, int(n)):
            nxt = prev + curr
            print str(curr),
            prev = curr
            curr = nxt
    except ValueError or Exception:
        new = raw_input("Invalid input. Please enter a positive integer: ")
        fib(new)

n = raw_input("Enter number of terms: ")
fib(n)
bananamuffin
  • 301
  • 1
  • 5
  • 10
  • 2
    You should move the test for negative numbers *outside* the `try` - you *want* it to cause an error! Also note that `try` blocks should be short as possible, and `except` tests as specific as possible, otherwise you hide genuine problems. – jonrsharpe Jul 06 '16 at 08:09

4 Answers4

4

As Jon says, it's a much better design strategy to separate the input gathering from the core Fibonacci calculation. Also, it's generally better to use a simple loop than to use a recursive call (calling a function inside itself) unless you really need recursion (eg, when processing a recursive data structure, like a directory tree).

Here's a modified version of your code. I've made a few other minor changes. In Python, we don't need a temporary variable like nxt to do the core Fibonacci calculation. Instead, we use tuple assignment so we can update curr and save the old curr to prev in one step.

def input_positive_integer(prompt=''):
    """ Get a positive integer from the user """
    while True:
        try:
            n = int(raw_input(prompt))
            if n <= 0:
                raise ValueError
            break
        except ValueError:
            print "Invalid input.",
            prompt = "Please enter a positive integer: "
    return n

def fib(n):
    """ Print n terms of the Fibonacci sequence """
    prev, curr = 0, 1
    for terms in range(n):
        print curr,
        prev, curr = curr, prev + curr

n = input_positive_integer("Enter number of terms: ")
fib(n)

test

Enter number of terms: -4
Invalid input. Please enter a positive integer: q
Invalid input. Please enter a positive integer: 2.5
Invalid input. Please enter a positive integer: 5
1 1 2 3 5
PM 2Ring
  • 54,345
  • 6
  • 82
  • 182
  • quick question: when you said 'int(raw_input(prompt))' were you converting the input to an integer? Or were you validating the type of the input – bananamuffin Jul 06 '16 at 09:06
  • `raw_input(prompt)` always returns a string. `int(some_string)` attempts to convert `some_string` to an integer, but if `some_string` isn't a valid string representing an integer `int` will raise `ValueError`. So in the expression `int(raw_input(prompt))`, `int` is handling both the conversion and the validation for us. And once we've created the integer `n` we can then go on to verify that `n` is a non-negative integer. – PM 2Ring Jul 06 '16 at 10:01
2

except ValueError or Exception

The correct syntax for that is except (ValueError, Exception), but note that since all exceptions inherit from Exception, this might misbehave.

You better not raise and try to catch a general exception. Instead, raise the already catched ValueError:

def fib(n):
    try:
        if n <= 0:
            raise ValueError
    .
    .
    .
    except ValueError:
        .
        .
        .
DeepSpace
  • 78,697
  • 11
  • 109
  • 154
1

I think the core problem here is that your fib function has two entirely separate responsibilities:

  1. Handling user input and validation; and
  2. Calculating Fibonacci numbers.

From the name, I'd say it should only be doing #2!

Instead, structure your program like:

def positive_integer():
    """Loops until the user enters a positive integer."""
    ...

def fib(n):
    """Prints first n Fibonacci numbers."""
    if n < 0:
        raise ValueError
    prev = 0
    curr = 1
    for terms in range(n):  # note simplification here
        nxt = prev + curr
        print str(curr),
        prev = curr
        curr = nxt

fib(positive_integer())

Now you have two functions, each with a clear single responsibility. This makes each one less complex; the range call is now simpler, for example, as it can assume n is already an integer (and if not, the user gets a sensible TypeError to tell them what went wrong).

For writing positive_input see Asking the user for input until they give a valid response; a loop is better than recursion for this.

As a further refactor, you can actually cut the three lines

nxt = prev + curr
prev = curr
curr = nxt

to just one:

prev, curr = curr, curr + prev

Also note that, in general, you should keep your try blocks as short as possible and your except clauses as specific as possible. This makes them more useful when reading the code ("I think this one thing could go wrong, and in that case we can do this about it") and means that legitimate problems aren't quietly ignored. In particular, you should almost never be dealing directly with Exception (either in raise or except), it's far too broad a case to handle usefully.

Community
  • 1
  • 1
jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
0

To catch multiple exception types, put them in a tuple. ValueError or Exception is a boolean expression that evaluates to ValueError.

Also you should never raise Exception, and you should never ever catch it. Define your own custom exception class.

class NonNegativeNumber(Error):
    pass


...
except (ValueError, NonNegativeNumber):

However, note that ValueError seems like a perfectly valid exception to raise here; you may not need the more specific class at all.

Daniel Roseman
  • 588,541
  • 66
  • 880
  • 895
  • `ValueError or Exception` is a boolean expression that evaluates to `ValueError` (as I'm sure you well know). – Duncan Jul 06 '16 at 08:12