1
\$\begingroup\$

I just started coding programs with python. As a challenge I wanted to program something to print out the Fibonacci Numbers. It looks like I've done what I wanted to do (I checked the first few numbers) and because I just started coding, I obviously don't know if this is a legitimate code and somebody would code it like that. Keep in mind that I have only done some basic lessons in python so I just want to get some feedback and advice for better code.

def spiral(sequence):
    numbers = []
    for i in sequence:
        numbers.append(0)
        if i == 0 or i == 1:
            numbers[i] = 1
            continue
        numbers[i] = numbers[i-1] + numbers[i-2]
    return numbers
times = range(int(input("How many times do you want to go through? ")))
fibonacci = spiral(times)
print(fibonacci)
\$\endgroup\$
3
\$\begingroup\$

The way sequence is used guarantees that the results will be wrong for the majority of inputs. It's clearly intended that sequence should be a continuous range starting at 0. So rather than require the range as input, it would make more sense to require the limit as input.


        numbers.append(0)
        if i == 0 or i == 1:
            numbers[i] = 1
            continue
        numbers[i] = numbers[i-1] + numbers[i-2]

This confused me briefly until I figured out that it appends 0 as a placeholder and then overwrites it with the appropriate Fibonacci number. Following the principle of KISS (keep it simple), it would make more sense to append the Fibonacci number directly.

IMO the expression is short enough to use a ternary:

        numbers.append(numbers[i-1] + numbers[i-2] if i > 1 else 1)

Those two changes give

def spiral(n):
    numbers = []
    for i in range(n):
        numbers.append(numbers[i-1] + numbers[i-2] if i > 1 else 1)
    return numbers
times = int(input("How many times do you want to go through? "))
fibonacci = spiral(times)
print(fibonacci)

The names seem quite opaque. Why spiral? Why times? They both seem to assume some context which isn't documented in the code.


It would be more Pythonic to write a generator, but since you say you're a beginner this may be a concept which you haven't encountered yet.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback. The names of the variables came from translating the variable names from my language to english, which wasn't very smart to make them in my language I know, but I had do think of names quick. Gonna work on the naming next time. :) \$\endgroup\$ – nookyi Jun 30 '18 at 12:44

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.