cannot figure out why my letters are not rotating in the Caesar code in Python

Question

I have been working on this problem for about a week now and cannot figure out why my letters are not rotating. I am NOT looking for someone to do the code for me but rather help me figure out where my problems are. Basically, I am looking for a rubber duck to help with this problem. I have seen it rotate the last letter but not the rest of the letters. Here is where I am at right now.

from string import ascii_uppercase, ascii_lowercase

def caeser_encrypt(string, step):
    new_string = list(string)

    for i in range(len(new_string)):
        new_ascii = ord(new_string[i]) + step

    if string[i] in ascii_uppercase:
        if new_ascii > 90:
            new_ascii = new_ascii - 90 + 64
        elif new_ascii < 65:
            new_ascii = 91 - 65 - new_ascii

    if string[i] in ascii_lowercase:
        if new_ascii > 122:
            new_ascii = new_ascii - 122 + 96
        elif new_ascii < 97:
            new_ascii = 123 - 97 - new_ascii
        new_string[i] = chr(new_ascii)

    return ''.join(new_string)

def main ():
    string = input('Enter word(s)')
    step = input("How many rotations do you want?")
    step = int(step)

    print(caeser_encrypt(string, step))

if __name__ == "__main__":
    main()

Answer 1

Only the last letter is rotated, because of this indentation:

for i in range(len(new_string)):
    new_ascii = ord(new_string[i]) + step

if string[i] in ascii_uppercase:
    if new_ascii > 90:
        new_ascii = new_ascii - 90 + 64
    elif new_ascii < 65:
        new_ascii = 91 - 65 - new_ascii

What you intended is this:

for i in range(len(new_string)):
    new_ascii = ord(new_string[i]) + step

    if string[i] in ascii_uppercase:
        if new_ascii > 90:
            new_ascii = new_ascii - 90 + 64
        elif new_ascii < 65:
            new_ascii = 91 - 65 - new_ascii

That is, the if statements should be indented under the for statement. If the if statements are not indented like that, then they are not executed in a loop, but executed only once, after the for loop. At that time i is set to the index of the last letter, that's why only the last letter is rotated.

Code review

Many other improvements are possible.

Instead of magic numbers like 90, 65, it would be better to use ord('z') and ord('a').

The if string[i] in ascii_uppercase and if string[i] in ascii_lowercase are mutually exclusive conditions, so they should be chained together with elif.

Instead of if string[i] in ascii_uppercase, which performs a linear search in ascii_uppercase (checks each value in ascii_uppercase until it finds a match), it would be more efficient to use a range check, if 'A' <= string[i] <= 'Z'.

The implementation replaces non-alphabetic characters too. If I want to rotate "hello world", that will give a funny result, because of the space. Maybe that's ok like that though, so that word boundaries remain indistinguishable. So that's not a criticism, just a side note.

Putting it together, and some other minor improvements, you can write like this:

def caeser_encrypt(string, step):
    new_string = list(string)

    for i, c in enumerate(new_string):
        new_ascii = ord(c) + step

        if 'A' <= c <= 'Z':
            new_ascii = ord('A') + (new_ascii - ord('A')) % 26

        elif 'a' <= c <= 'z':
            new_ascii = ord('a') + (new_ascii - ord('a')) % 26

        new_string[i] = chr(new_ascii)

    return ''.join(new_string)