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
asked Nov 30, 2017 at 8:30
\$\endgroup\$

3 Answers 3

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)
answered Nov 30, 2017 at 9:10
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Minor point. I think the docstring should mention how the function behaves for invalid inputs, i.e. raises an error. \$\endgroup\$ Commented Nov 30, 2017 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
answered Nov 30, 2017 at 10:18
\$\endgroup\$
11
  • \$\begingroup\$ I think by avoiding to use the modulus operand, you make it progressively difficult for yourself. Much more then need be. \$\endgroup\$ Commented Nov 30, 2017 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\$ Commented Nov 30, 2017 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\$ Commented Nov 30, 2017 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\$ Commented Nov 30, 2017 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\$ Commented Nov 30, 2017 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.

answered Nov 30, 2017 at 15:57
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.