[Tutor] implementing rot 13 problems

[Steven D'Aprano]

On 12/03/13 16:57, RJ Ewing wrote:
> I am trying to implement rot 13 and am having troubles. My code is as
> follows:

A few comments follow.


>   def rot_text(self, s):
>       ls = [i for i in s]
>       for i in ls:
>           if i.isalpha():
>               if i.isupper():
>                   if i <= 'M':
>                       x = ls.index(i)
>                       ls[ls.index(i)] = chr(ord(i) + 13)


This is unnecessarily verbose and complicated.

First problem, you use the name "i" to represent a character. Generally you should avoid single character names, and prefer meaningful names that explain what the variable represents. If you must use a single character, then the conventional names include:

x, y, z: numbers, particularly floats
x: (rarely) an arbitrary value of no particular type
i, j, k: integer loop variables (rare in Python)
n, m: other integers
s: string
c: character

Notice that "c for character" makes much more sense than "i for character".


You can, and should, create a list from the string using the list() function, rather than a list comprehension that just walks over the characters:

ls = [c for c in s]  # No, too verbose.
ls = list(s)  # Much better.


You then loop over the list, again using "i for character". You can combine all those multiple if statements into a single clause:

       for i in ls:
           if i.isalpha() and i.isupper() and i <= 'M':

But wait! The call to isalpha() is not necessary, because if a character is *not* alphabetical, isupper() will always return False:

py> '9'.isupper()
False

So we can throw out the call to isalpha().

Your code to rot13 the letter is, sadly, broken:

x = ls.index(i)
ls[ls.index(i)] = chr(ord(i) + 13)


This might make more sense if I re-write it using more meaningful names:

position = ls.index(char)  # This variable never gets used :-(
ls[ls.index(char)] = chr(ord(char) + 13)

The bit with chr() and ord() is fine. But what you do with it is wrong. The problem is that index always returns the position of the *first* copy of the item. So if your string is:

'ABA'

the first time around the loop, your character is 'A'. ls.index('A') will return 0, and the rot13ed version, 'N', will be shoved into position 0. The second time around the loop, the character is 'B' and the rot13ed version gets shoved into position 1. But the third time, the character is 'A' again, index() will return 0 again, and the rot13ed version gets shoved into position 0 instead of position 2, giving you:

'NOA'


The best way to fix this is to ignore index, and instead count which character we're at each time:


position = -1
for char in ls:
     position = position + 1  # Advance the count.
     ... blah blah blah
     ls[position] = chr(ord(char) + 13)


But wait! Python has a function to do exactly this for us:


for position, char in enumerate(ls):
     ... blah blah blah
     ls[position] = chr(ord(char) + 13)




Have a go with that, and see how far you get.




-- 
Steven