3
\$\begingroup\$

Just finished an assignment in Python where one part was to write a function to find the correct rotation in a encryption similar to a Caesar Cipher..

As mentioned it's already finished and works as intended. The course is an introduction to Python, but I do have experience with other languages since previously.

I was hoping someone could have a look at my findRot to see where I possibly could have done anything that isn't 'Python-esque' or what potential bottlenecks might occur.

def findRot(word1, word2, iterator):
    letterFreq = [("A", 65), ("E", 69), ("I", 73), ("O", 79), ("T", 84), ("U", 85)]
    rot = ord(word1) - letterFreq[iterator][1]
    letters = [chr(ord(letter) - rot) if (ord(letter) - rot >= 65) else chr(90 - (65 -(ord(word2[2]) - rot + 1))) for letter in word2]
    with open('words.txt', 'r') as words:
        lines = words.readlines()
        lines = list(map(lambda s: s.upper().strip(), lines))
        if ''.join(letters) in lines:
            print(''.join(letters) + ", " + str(True))
            return rot
        else:
            findRot(word1, word2, iterator + 1)

The assignment specified the usage of letter frequency, so I just took some of the more common letters found in English and put them in a list of tuples for readability purposes.

In the professor's use-case the first word would be 'R' and the second would be 'Uxen'. So in this case the rotation would supply the 'I' and 'Love'.

Again, this function just runs a primary check to find the rotation to apply to the rest of the 'encrypted' text. So a validation of the decrypted text will take place later.

EDIT:

Example usage:

>>> findRot('R', 'UXEN', 0)
9

Because it takes 9 rotations to get to the clear text I and Love.

\$\endgroup\$
3
\$\begingroup\$

First off, you should follow PEP8. This will make your code more readable for Python programmers, and there is quite a few tips in there to make your code more Pythonic. The amount of whitespace that you use is actually pretty good! It's mostly your names that 'let you down'. But you're following a convention of camelCase. Which is also ok, but snake_case is preferred.

I don't like the re-definition of letterFreq and lines on ever function call. This leads to a performance hindrance. And so I'd move them out of the function to make them global. Also as they are constants you may want to follow PEP8 and name them LETTER_FREQ and WORDS.

After this I'd look into removing the recursion. In Python recursion is cute, and un-reliable, nothing more. I'd recommend that you don't use it, and instead you can use a for loop. Using range starting at iterator and ending at len(LETTER_FREQ) we can remove the need for the recursion.

I'd also remove the print, instead of calling it in the function you should do it outside of the function. If I'm using this function I don't want a print there ever couple of minutes.

Finally your turnery is a bit awkward and so I'd change it to use the modulo function, %. This allows you to remove the if/else as -1 % 26 == 25 and so we can replace it with chr((ord(letter) - rot - 65) % 26 + 65).

Adding all the above together lead me to:

LETTER_FREQ = "AEIOTU"

with open('words.txt', 'r') as f:
    WORDS = [word.upper().strip() for word in f.readlines()]

def find_rotation(word1, word2, iterator=0):
    for index in range(iterator, len(LETTER_FREQ)):
        rot = ord(word1) - ord(LETTER_FREQ[index])
        letters = ''.join([chr((ord(letter) - rot - 65) % 26 + 65) for letter in word2])
        if letters in WORDS:
            return rot
    return None

For an additional speed you can also change WORDS to be a set, WORDS = set(WORDS), this is as it'll then be \$O(1)\$ look up in the if letters in WORDS: line, rather than \$O(n)\$. This is good as if the words list is large then it'll take a significantly larger amount of time then it would with a set.

\$\endgroup\$
  • \$\begingroup\$ There seems to be an error where the function returns NoneType when I apply your function. Otherwise it seems like legit advice. I'm trying my way forward whilst using Python. I come from a .NET background and currently work as a .NET developer, so I really appriciate your advice. \$\endgroup\$ – Todai Sep 16 '16 at 15:50
  • \$\begingroup\$ @Todai Yes you're correct, I accidently used iterator rather than index to index LETTER_FREQ. I've tested the above with your example and it works A-OK. Also your code's pretty good, don't worry about it. \$\endgroup\$ – Peilonrayz Sep 16 '16 at 16:08
  • \$\begingroup\$ a function returns None by default when it finishes, so there is no need to do it explicitly (even though explicit is better than implicit...). 26 and 65 also look like magic constants (26 == len(alphabet)?), so I would give them a name. \$\endgroup\$ – Graipher Sep 17 '16 at 9:31
  • \$\begingroup\$ @Graipher Your point about return None, I was following PEP8: "If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None , and an explicit return statement should be present at the end of the function (if reachable)." I don't really mind 26 and 65, being 'magical', you can make an answer, if you want, about magic constants and mention OPs 65, 90 and 1. \$\endgroup\$ – Peilonrayz Sep 17 '16 at 12:19
2
\$\begingroup\$

This code just doesn't make sense, even after your added explanation. In fact, I am skeptical that this function does what it is intended to do. ord(word1) works only if word1 is a single character; anything longer causes

TypeError: ord() expected a character, but string of length n found

Is there a second example call, other than findRot('R', 'UXEN', 0), that demonstrates how this function accomplishes anything useful? I doubt it.

Another weird thing is that letterFreq is not a table of letter frequencies, as one would expect. Rather, it is a table of ASCII codes for six letters. Why do you need such a table? Why those six letters?

The iterator parameter is mysterious too. Apparently, it needs to be an integer; it's not an iterator as one would expect from the name. What is it for? Should the function always be called with 0 as the third argument? It seems like a clunky design.

I don't believe that recursion is a good idea here. The most significant problem I see is that words.txt is opened and read with every call of this function — a horrible inefficiency when the function is recursive.

\$\endgroup\$

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.