I need some help condensing my Caesar Cipher (encryption and decryption).


key = 8
f = open('unencrypted.txt','r+')
lower = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"

g = open('encryptedmessge','w')
for line in f:
    s = list(line)
    for i in range(0,len(s)):
        if lower.find(s[i],26) != -1:
            s[i] = lower[lower.find(s[i])+key]

        if upper.find(s[i],26) != -1:
            s[i] = upper[upper.find(s[i])+key]
line = ''.join(s)


I want to make the code less sloppy, maybe not so many if statements? A more efficient decryption.

f = open('undecrypted.txt','r+')
lower = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"
alph =       dict(a=0,b=0,c=0,d=0,e=0,f=0,g=0,h=0,i=0,j=0,k=0,l=0,m=0,n=0,o=0,p=0,q=0,r=0,    s=0,t=0,u=0,v=0,w=0,x=0,y=0,z=0)
g = open('decryptmessage','w')
for line in f:
    s = list(line)
    for i in range(0,len(s)):
        if lower.find(s[i],26) != -1:
            alph[s[i]] += 1
        if upper.find(s[i],26) != -1:
max = 0
for k,v in alph.items():
    if v > max:
        max = v
        maximum = k
key = lower.find(maximum) - lower.find('e')
f = open('undecrypted.txt', 'r+')
for line in f:
    s = list(line)
    for i in range(0,len(s)):
        if lower.find(s[i],26) != -1:
            if lower.find(s[i])-key > 0:
                s[i] = lower[lower.find(s[i])-key]
                s[i] = lower[26+(lower.find(s[i])-key)]
         if upper.find(s[i],26) != -1:
            if upper.find(s[i])-key > 0:
                s[i] = upper[upper.find(s[i])-key]
                s[i] = upper[26+(upper.find(s[i])-key)]
line = ''.join(s)
print line

3 Answers 3

from collections import Counter
from string import ascii_lowercase, ascii_letters

def get_cypher_key(filename, key_letter='e'):
    letters = Counter()
    with open(filename) as cypher_file:
        for line in cypher_file:
    for symbol in set(letters) - set(ascii_lowercase):
        del letters[symbol]
    most_common_letter, _ = letters.most_common(1)[0]
    return ord(most_common_letter) - ord(key_letter.lower())

def cypher_ascii(letter, key):
    if letter not in ascii_letters:
        return letter
    if letter.islower():
        lower, upper = ord('a'), ord('z')
        lower, upper = ord('A'), ord('Z')
    cypher = ord(letter) + key
    if cypher < lower: cypher += 26
    if cypher > upper: cypher -= 26
    return chr(cypher)

def uncypher_text(filename, key_letter='e'):
    key = get_cypher_key(filename, key_letter)
    return cypher_text(filename, -key)

def cypher_text(filename, key):
    with open(filename) as uncyphered_file:
        for line in uncyphered_file:
            yield ''.join(cypher_ascii(symbol, key) for symbol in line)

if __name__ == "__main__":
    print('Cyphering "unencrypted.txt":')
    with open('encryptedmessage', 'w') as message_file:
        for cyphered in cypher_text('unencrypted.txt', 8):

    print('Decyphering "undecrypted.txt":')
    with open('decryptedmessage', 'w') as message_file:
        for decyphered in uncypher_text('undecrypted.txt'):

Not sure if it is really condensed but it is less sloppy. The key is to use functions that you can easily test and reuse.

Some points on your code:

  • use open with with so that forgetting to close the file as you did won't be a problem;
  • do not use max or other builtin functions names as variable names, it's error prone;
  • do not build ASCII letters strings yourself, they already are in the string module;
  • Counter should be an obvious choice when counting things, or at least defaultdict(int)

If you wonder about the yield keyword in my code, you can read about it on stackoverflow.


As I added the encryption part of your code, you can see how much it can get improved by the use of helper functions. Mostly because the encryption and decryption of Cæsar Ciphers are symmetrical operations.



  • Use better names, s, g, f are all bad names. unencrypted_file for instance a good name.

  • You should have more white-space in some areas, range(0,len(s)) -> range(0, len(s)).
    There is one main exception to this, and that's if you want to show precedence 1 + 1 * 2 and 1 + 1*2 are both good.

  • Use with to open files, e.g. with open('unencrypted.txt','r+') as f:

  • When iterating through things that need both the index and value, use enumerate, as it is generally a good idea.

  • Use functions, def encrypt(data, shift) would allow your code to be easier to understand and to re-use.

  • Use % to limit the range. (26 + 8) % 27 == 8. It also works for negatives! (0 + -3) % 27 == 24.

  • Use in to check if an item is in another item. s[i] in lower.

  • There seems to be a bug in your encrypt. You only save the encrypted last line.

  • There is either a bug, or waist of CPU happening in your decryption, if upper.find(s[i],26) != -1:alph[s[i].lower()]. I assume this is meant to be alph[s[i].lower()] += 1.

  • Assuming you are a beginner, and don't care about the included battery's in Python, you can get the max value in Python with max. E.g. max(dict.iteritems(), key=(lambda item: item[1])).

  • You can 'pre-calculate' lower.find('e'), which is 4.

  • You decrypt with the -ve of the encrypt. So decrypt can simply be def decrypt(data, key):return encrypt(data, -key).

I use the above in the below, and don't improve apon your dict. I would encourage you read @MathiasEttinger's answer about improving dict.

import strings

lower = string.ascii_lowercase
upper = string.ascii_uppercase
ascii = string.ascii_letters

def encrypt(data, key):
    buff = list(data)
    for index, char in enumerate(buff):
        if char in ascii:
            letters = lower if char in lower else upper
            buff[index] = letters[(letters.find(char) + key) % 27]
    return ''.join(buff)

def decrypt(data, key):
    return encrypt(data, -key)

def assume_key(file_handler, letter='e'):
    alph = dict(a=0,b=0,c=0,d=0,e=0,f=0,g=0,h=0,i=0,j=0,k=0,l=0,m=0,n=0,o=0,p=0,q=0,r=0,s=0,t=0,u=0,v=0,w=0,x=0,y=0,z=0)

    for line in file_handler:
        for char in line:
            alph[char.lower()] += 1

    maximum = max(dict.iteritems(), key=(lambda item: item[1]))[0]
    return lower.find(maximum) - lower.find(letter)

# Encrypt file
key = 8
with open('unencrypted.txt','r+') as unencrypted_file,
     open('encryptedmessge','w') as encrypted_file:
    for line in unencrypted_file:
        encrypted_file.write(encrypt(line, key))

# Decrypt file
with open('undecrypted.txt','r+') as undecrypted_file:
    assumed_key = assume_key(undecrypted_file)

with open('undecrypted.txt','r+') as undecrypted_file,
     open('decryptmessage','w') as decrypt_file:
    for line in undecrypted_file:
        decrypt_file.write(decrypt(line, assumed_key))

There may still be a few errors with file handling.

Also, I changed lower.find(maximum) - lower.find('e') as whilst 'e' is the most common letter in English, you can do file_data.replace('e', '') or purposely leave of the 'e's so that your decrypt will not work. To fix this you may want to append the most common letter to the end of the encrypted file or something.


Your code seemed to be unneccessarily complicated, and the main issue is that you are indexing a lot of lists, instead of using the elegant pythonic approaches for reading lines and characters in lines. Compare these two up against each other:

# Using indexes
for line in f:
    s = list(line)
    for i in range(0, len(s)):
        print s[i]

# Versus automated splitting of lines
for line in f:
    for c in line:
        print c

Another similar optimisation is that you change the array, s, and then join it at the end, this complicates matters also. If possible, try to use list comprehensions, i.e. encoded_line = ''.join(encode(c) for c in line). This makes use of a function to encode a given character.

The encryption and decryption are quite similar, so I suggest joining these into one. You'll also have a bug in your code, related to how to find the key (or offset) to use when decrypting. Trusting that e is the most common character is at best dangerous. It is better either to include it somehow in the file, or let it be a shared secret between encoder and decoder what the offset is supposed to be.

One safer option, if you don't want to have a shared secret, could be to use the length of the first word, or to insert the key as first/last character of file. But depending on frequency of a given character is not a good solution.

Using better variable names is also good practice. Avoid single character names for anything but loop indexes like i (and possibly c for a character). Making use of predefined tables are also a benefit you can take advantage off.

The final note, as some other have commented, is to make use of the with syntax for opening of files to help encapsualte file reads and writes in a nice way.

Revamped code

Here is my revamped version of your code, complete with an example of encryption, decryption and comparison of the file. Has left a commented print statement, if you want to see the line read, and encrypted version of it.

Just for the fun it, I also added in the character classes of digits and punctuation, to further scramble the encrypted file, and if a character is not in any of the character classes it is kept as is.

Here is the code:

from string import ascii_lowercase, ascii_uppercase, digits, punctuation
from operator import add, sub
import filecmp

def cipher_file(input_filename, output_filename, offset=8, encrypt = True):
    """En-/decrypt filename using caesar cipher with given key."""

    with open(input_filename, "r") as input_file, open(output_filename, "w") as output_file:
         # Encrypt all the lines, and output to file
         for line in input_file:
             encoded_line = ''.join(cipher_character(c, offset, encrypt) for c in line)

             # print("{}{}".format(line, encoded_line))

def cipher_character(character, offset=8, encrypt=True):
    """En-/decrypt a single characer with given key."""

    # When encrypting we add the offset, and else we subtract the offset
    operator = add if encrypt else sub

    # Check if the character is in a character_table, and if so return the
    # character rotated by that offset
    for character_table in (ascii_lowercase, ascii_uppercase, digits, punctuation):
        if character in character_table:
            return character_table[operator(character_table.index(character), offset) % len(character_table)]

    # If not found in any of the character tables, return the plain character
    return character

def main():
    # Encrypt the file
    cipher_file("test_data/input_file", "test_data/encrypted_file", 15)

    # Decrypt the file
    cipher_file("test_data/encrypted_file", "test_data/output_file", 15, False)

    # Compare input and output files
    print("Are files equal? {}".format(filecmp.cmp("test_data/input_file",

if __name__ == '__main__':

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.