1
\$\begingroup\$

I was inspired by a solution in Think Python: How to Think Like a Computer Scientist. I structured mine (into two functions) according to it, and used the technique used in rotate_letter. It's here if you're interested.

How can I make this better in all aspects (documentation, algorithm, variable names, and etc.)? Is memoizing rotated_string worth it?

def rotated_letter(letter, key):
    """Returns a single-letter string representing
       letter rotated by key, and returns letter if
       it doesn't belong to any of the English alphabet.

       letter: a single-letter string.

       key: an integer.
       """
    if letter.isupper():
        start = ord('A')

    elif letter.islower():
        start = ord('a')

    else:
        return letter

    diff = ord(letter) - start
    position = (diff + key) % 26 + start

    return chr(position)

def rotated_string(string, key, encrypt=True):
    """Returns string rotated by key.

       string: a string.

       key: an integer.

       encrypt: a boolean value.
       """
    result = ''
    memo = {}

    if not encrypt:
        key = -key

    for character in string:
        if character in memo:
            # rot_cahr denotes rotated character
            rot_char = memo[character]
        else:
            rot_char = memo.setdefault(character, rotated_letter(character, key))

        result += rot_char


    return result

def main():
    print(rotated_string('Hello!', 3))
    print(rotated_string('Khoor!', 3, encrypt=False))

if __name__ == '__main__':
    main()
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Docstrings: If using (and only targeting) python 3.5 or later, you could remove the type hints from the docstring and include them as real type hints. You could also extend the docstrings to contain a (doctest-conforming) example usage.

Algorithm: Unicode is HARD. Python3 strings are unicode. The caesar cipher only works for letters that are in ascii range. Example:

>>> rotated_letter("Á", 13)
'L'
>>> rotated_letter("L", -13)
'Y'

So your test by isupper / islower is not enough. You could do 'a' <= letter <= 'z' for an ascii lowercase test.

Variable names: Just a gut feeling: functions do stuff. So a good name for a function is an imperative: I'd use rotate_letter instead of rotated_letter. Also, once you name the functions rotate_xxx, the concept of a key is not valid anymore (this belongs into functions like encrypt and decrypt). When speaking of rotation, I'd call the argument offset or something like that.

Memoization / Performance: The letter encryption function does not do much, so memoization is probably not necessary. You are concatenating strings for building the result. This is slow. If this slowness doesn't matter to you (because of your small-input use case), then memoization is overkill. If you want better performance, you would (in the general case) append your string parts onto a list, and join the list when the calculation is done. In this special case, I would even opt for a preallocated array, as the output size is known.

PS: I learned something, too, i.e. that there is setdefault() for dicts. Thanks!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.