6
\$\begingroup\$

The code calculates a n-bonnaci sequence to a certain number, based on the user's input. It then can print it to a file and/or print it to the console.

The calculateSequence function calculates the sequence. The first loop fills the sequence with ones (0, 1, 1, 2...). Next, it calculates the value of the current number, using crrntVlue as a placeholder, and using i as the location of the value to add (a number is the sum of the preceding two numbers). sqnceItratr is location of the number that is being calculated.

My questions are:

  1. Is there anything immediately wrong that you see in the code?
  2. How can I make the code more readable/better variable names, as it was hard to understand the code while coding it?
  3. How can I restructure the program to make it better?

This is one of my first Python programs, so I want to know what I might be coding 'wrong'.

def calculateSequence(n, amntToCalc):
    sqnceItratr = 1
    sequence = [0] * numbrsToCalc

    #Add 1's to the array(Duonacci is 1, 1, 2... Tribonacci is 1, 1, 1, 3...)
    while sqnceItratr < n:
        sequence[sqnceItratr] = 1
        sqnceItratr += 1

    #Actually Calculate the Sequence
    while sqnceItratr < numbrsToCalc:
        i = N
        #i is the location of the amount to add to crrntVlue
        #Ex: sequence[3] = 1 + 1 + 1 = 3, i is sequence[3 - 1]

        crrntVlue = 0
        while i > 0:
            #This is to calculate the actual value of a number
            #in the sequence
            crrntVlue += sequence[sqnceItratr - i]
            i -= 1
            #Count Down for the number to add to crrntValue(n-bonnacci)

        sequence[sqnceItratr] = crrntVlue
        # Add crrntValue to the array
        sqnceItratr += 1
        #Continue to the next value in the array
    return sequence

def convertInput(inputToConvert):
    inputToConvert = inputToConvert.upper()
    if inputToConvert == 'Y':
        return True
    elif inputToConvert == 'YES':
        return True
    else:
        return False

N = int(input("What do you want to n in N-bonnacci to be? \n"))
numbrsToCalc = int(input("How many digits do you want to calculate? \n This should probably be bigger than n \n"))
sequence = calculateSequence(N, numbrsToCalc)

userInput1 = convertInput(input("Do you want to write it to a file? \n"))
sequencePosition = 0
if userInput1 == True:
    file = open("Seqence.txt", "w")
    while sequencePosition < len(sequence):
        file.write(str(sequencePosition + 1) + ': ' + str(sequence[sequencePosition]) + '\n')
        sequencePosition += 1
    file.close()

userInput2 = convertInput(input("Do you want to print it out? \n"))
if userInput2 == True:
    sequencePosition = 0
    while sequencePosition < len(sequence):
        print(str(sequencePosition + 1) + ': ' + str(sequence[sequencePosition]))
        sequencePosition += 1
| improve this question | |
\$\endgroup\$
  • \$\begingroup\$ If this is not actually the case, I suggest you use Pylint in order to check if your code is respecting the coding standards of Python, amongs other things. see pylint.org \$\endgroup\$ – Hello_world Feb 10 '16 at 9:25
5
\$\begingroup\$

  • Algorithm

    Looks like you are doing too much. The recurrence can be greatly simplified:

    \$a_{n} = a_{n-1} + a_{n-2} + \cdot\cdot\cdot + a_{n-k+1} + a_{n-k}\$, and

    \$a_{n+1} = a_{n} + a_{n-1} + \cdot\cdot\cdot + a_{n-k+1}\$, so

    \$a_{n+1} = a_{n} + (a_{n-1} + \cdot\cdot\cdot + a_{n-k+1}) = a_{n} + (a_{n} - a_{n-k})\$

    meaning that the next k-bonacchi number is just

      sequence[n+1] = 2*sequence[n] + sequence[n-k]

  • Naming

    Honestly, names like sqnceItratr and crrntVlue are more than unreadable. Don't abbreviate so aggressively. In this particular case I'd say that n fits much better than sqnceItratr.

  • Separation of concerns

    calculateSequence initialize the sequence and produces the rest of it in a single call. It doesn't look right. I recommend to split the solution in initialize, and compute_next calls, along the lines of

    sequence = initialize(N)
for n in range(N, limit):
    value = compute_next(sequence, N)
    sequence.append(value)
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So sequence[2 + 1] = 2 * sequence[2] + sequence[n-k], sequence[3] = 2 * 1 + 2 - 2, if the sequence is 0, 1, 1, 2? \$\endgroup\$ – Jacky Xie Feb 8 '16 at 12:36
2
\$\begingroup\$

Let's take a quick look.

def calculateSequence(n, amntToCalc):
    sqnceItratr = 1

Why are you starting iteration at 1 instead of 0?

    sequence = [0] * numbrsToCalc

amntToCalc is a bit short. Maybe write result_length or something?

    #Add 1's to the array(Duonacci is 1, 1, 2... Tribonacci is 1, 1, 1, 3...)
    while sqnceItratr < n:
        sequence[sqnceItratr] = 1
        sqnceItratr += 1

This loop can be replaced with sequence[1:n] = [1] * (n - 1).

    sequence[1:n] = [1] * (n-1)
    sqnceItratr = n

A bit shorter. (I'm hoping to get rid of the sqnceItratr later on).

    #Actually Calculate the Sequence
    while sqnceItratr < numbrsToCalc:
        i = N
        #i is the location of the amount to add to crrntVlue
        #Ex: sequence[3] = 1 + 1 + 1 = 3, i is sequence[3 - 1]

        crrntVlue = 0
        while i > 0:
            #This is to calculate the actual value of a number
            #in the sequence
            crrntVlue += sequence[sqnceItratr - i]
            i -= 1
            #Count Down for the number to add to crrntValue(n-bonnacci)

Let's look if we can get rid of this loop a bit.

        crrntVlue = sum(sequence[sqnceItratr - N:sqnceItratr])

Glad we got rid of that loop, and replaced it with a simple sum function. Also, N is a global variable, we should replace that with n (which is passed in).

        crrntVlue = sum(sequence[sqnceItratr - n:sqnceItratr])
        sequence[sqnceItratr] = crrntVlue

We can get rid of the temporary variable.

        sequence[sqnceItratr] = sum(sequence[sqnceItratr - n:sqnceItratr])

Better. This leaves us with

def calculateSequence(n, amntToCalc):
    sqnceItratr = 1
    sequence = [0] * numbrsToCalc

    #Add 1's to the array(Duonacci is 1, 1, 2... Tribonacci is 1, 1, 1, 3...)
    sequence[1:n] = [1] * (n-1)
    sqnceItratr = n

    #Actually Calculate the Sequence
    while sqnceItratr < numbrsToCalc:
        sequence[sqnceItratr] = sum(sequence[sqnceItratr - n:sqnceItratr])
        # Add crrntValue to the array
        sqnceItratr += 1
        #Continue to the next value in the array
    return sequence

I'm not a huge fan of while loops myself.

    #Actually Calculate the Sequence
    for sqnceItratr in range(n, numbrsToCalc):
        sequence[sqnceItratr] = sum(sequence[sqnceItratr - n:sqnceItratr])
    return sequence

Even shorter. Now we can get rid of the sqnceItratr before the loop. Also drop the comments, because the code itself is now more clear.

def calculateSequence(n, amntToCalc):
    sequence = [0] * numbrsToCalc
    sequence[1:n] = [1] * (n-1)
    for sqnceItratr in range(n, numbrsToCalc):
        sequence[sqnceItratr] = sum(sequence[sqnceItratr - n:sqnceItratr])
    return sequence

Ok, the sqnceItratr could be renamed as well, but it's ok.

def convertInput(inputToConvert):
    inputToConvert = inputToConvert.upper()
    if inputToConvert == 'Y':
        return True
    elif inputToConvert == 'YES':
        return True
    else:
        return False

Oh, we can replace the two == with in ('Y', 'YES').

def convertInput(inputToConvert):
    inputToConvert = inputToConvert.upper()
    if inputToConvert in ('Y', 'YES'):
        return True
    else:
        return False

Even better: replace it with a simple return:

def convertInput(inputToConvert):
    return inputToConvert.upper() in ('Y', 'YES')

And now the calling code.

N = int(input("What do you want to n in N-bonnacci to be? \n"))
numbrsToCalc = int(input("How many digits do you want to calculate? \n This should probably be bigger than n \n"))
sequence = calculateSequence(N, numbrsToCalc)

userInput1 = convertInput(input("Do you want to write it to a file? \n"))
sequencePosition = 0
if userInput1 == True:
    file = open("Seqence.txt", "w")
    while sequencePosition < len(sequence):
        file.write(str(sequencePosition + 1) + ': ' + str(sequence[sequencePosition]) + '\n')
        sequencePosition += 1
    file.close()

userInput2 = convertInput(input("Do you want to print it out? \n"))
if userInput2 == True:
    sequencePosition = 0
    while sequencePosition < len(sequence):
        print(str(sequencePosition + 1) + ': ' + str(sequence[sequencePosition]))
        sequencePosition += 1

Variable names could be replaced: userInput1 is writeToFile, userInput2 is writeToStdout.

The code could be generalised out, writing to file and sys.stdout.

if writeToFile:
    with open('Seqence.txt', 'w') as f:
        writeSequence(sequence, f)
 if writeToStdout:
     writeSequence(sequence, sys.stdout)

(Also, instead of f = open(...) and f.close(), use with open(...) as f.).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ What does n:sqnceItrator mean?(Example 9, line 11) \$\endgroup\$ – Jacky Xie Feb 9 '16 at 3:33
  • \$\begingroup\$ @JackyXie: It's a 'slice' operator. It represents a sublist starting at the position of the first argument, and ending just before the last argument. If lst = [0, 4, 5, 9, 12], then lst[2:4] == [5, 9]. You can also assign to a slice: lst[2:4] = [8, 10] now gives lst = [0, 4, 8, 10, 12]. \$\endgroup\$ – Sjoerd Job Postmus Feb 9 '16 at 4:37
  • \$\begingroup\$ @Sjored I thought it was just for strings, forgot how strings and lists work... \$\endgroup\$ – Jacky Xie Feb 9 '16 at 12:53

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.