9
\$\begingroup\$

Could I have a couple people look over my code for a general readability check? Just make sure it is following Python standards.

def casear_cipher(letter, num):
    """ This function acts like a Casear Cipher. 

    Replaces an input letter with another letter a fixed

    number of spaces farther down the alphabet

    Args:

    * letter (string) - any upper case or lower case letter

    * num (integer) - any integer value to shift to a new letter
        """

    #Range 65 to 91 is ASCii values for uppercase alphabet
    if ord(letter) in range (65, 91):  
        x = ord(letter) - ord('A') 
        num_mod_26 = (x + num) % 26
        final_output = chr(ord('A') + num_mod_26)
        return final_output


    #Range 97 to 123 is ASCii values for lowercase alphabet 
    elif ord(letter) in range (97, 123):  
        x = ord(letter) - ord('a') 
        num_mod_26 = (x + num) % 26
        final_output = chr(ord('a') + num_mod_26)
        return final_output
\$\endgroup\$
11
\$\begingroup\$

First of all welcome to Codereview.

Good

  • You have a decent function.
  • You even have a nice docstring to describe what it does!
  • You have decent descriptive names.

Overall your code is good, well done!

Improvements

  • Make the starting_ascii value more generic, no more duplicated code.
  • Rais an appropriate exeption if no letter is given, you might want to handle that differently.

Revised code

def casear_cipher(letter, num):
    """This function acts like a Casear Cipher. 

    Replaces an input letter with another letter a fixed
    number of spaces farther down the alphabet

    Args:
    * letter (string) - any upper case or lower case letter
    * num (integer) - any integer value to shift to a new letter"""

    if letter.isupper():
        starting_ascii = ord('A')
    elif letter.islower():
        starting_ascii = ord('a')
    else:
        raise ValueError('Input is not a letter')

    alpha_index = ord(letter) - starting_ascii
    mod_26 = (alpha_index + num) % 26
    return chr(starting_ascii + mod_26)
\$\endgroup\$
  • 4
    \$\begingroup\$ Minor point. I think the docstring should mention how the function behaves for invalid inputs, i.e. raises an error. \$\endgroup\$ – Reti43 Nov 30 '17 at 15:17
2
\$\begingroup\$

This improvement is not related with readability, but I'm posting it because it can improve the encoding throughput for large text inputs.

Modulus operations are expensive for most integral values, because they require using more expensive instructions such as integer divisions or multiplications.

You can avoid this by just having your alphabet duplicated. The trick is that instead of wrapping the index you just unwrap the alphabet, since the range of a sum of two 0..26 values is always going to be less than 2*26. It would be a bit trickier but still possible, to support upper and lower case characters.

Here's a simplified code that could complement @Ludisposed answer:

from array import array
chars = range(ord('a'),ord('z')+1)
chars.extend(chars)
alphabet = array('c',(chr(c) for c in chars))

def caesar_cipher(letter, num):
   return alphabet[ord(letter)-ord('a')+num]

print(alphabet)
hello_encoded = "".join(caesar_cipher(c, 5) for c in "helloworld")
print(hello_encoded)

Result:

array('c', 'abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz')
mjqqtbtwqi
\$\endgroup\$
  • \$\begingroup\$ I think by avoiding to use the modulus operand, you make it progressively difficult for yourself. Much more then need be. \$\endgroup\$ – Ludisposed Nov 30 '17 at 10:26
  • \$\begingroup\$ You can use the modulus operand for sure, but if you plan to use it once for each character in a 10k character file, then you should think what matters more to you, your processing time or your debugging time. My main language is C, so you can imagine the source of my obsession for this kind of micro-optimizations :) \$\endgroup\$ – Jorge Bellon Nov 30 '17 at 11:29
  • 1
    \$\begingroup\$ I hope OP will not encode any data with this cypher, since Ceasar is a toy-grade Cypher. And unsuitable for actually encrypting data. :) \$\endgroup\$ – Ludisposed Nov 30 '17 at 11:35
  • 1
    \$\begingroup\$ And Donald Knuth's quote is usually misused. If you read the whole quote, he's actually talking about micro-optimizations in non critical code. \$\endgroup\$ – Jorge Bellon Nov 30 '17 at 11:44
  • 2
    \$\begingroup\$ This approach can simply be implemented with str.translate(), which can also handle non alphabetical characters by either leaving them untouched or removing them. \$\endgroup\$ – Reti43 Nov 30 '17 at 15:23
2
\$\begingroup\$

Very minor detail: your method name is spelled wrong. It's caesar, not cesaer, after the Roman general and politician who was one of the first recorded users of the cipher. You spelled it right in your question title, but not in your actual code. It's a small detail, but if the method is intended for consumption through an API or in other parts of your code, you may want to make sure that you don't have silly typographical mistakes like this, because fixing them is definitely not trivial, especially on products which have been released.

\$\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.