Vigenère cipher in Python

Question

Over the last year I have been programming on my own, mostly in Python. I haven't had any formal education, though I have followed some online instructions, and have been making things that interest me.

One of those things is my Vigenère encrypter. I've made a few versions of it, and I would appreciate feedback on my latest version. Such as, does it follow somewhat of a good design pattern for these things? How would it perform under heavy load?

If I can provide any additional information, please ask and I won't hesitate to do so.

This is my code:

from os import system
#characters to numbers table
dictDown = {'|':0,'a':1,'b':2,'c':3,'d':4,'e':5,'f':6,'g':7,'h':8,
 'i':9,'j':10,'k':11,'l':12,'m':13,'n':14,'o':15,'p':16,
 'q':17,'r':18,'s':19,'t':20,'u':21,'v':22,'w':23,'x':24,
 'y':25,'z':26,'.':27,',':28,"'":29,'!':30,':':31,'1':32,
 '2':33,'3':34,'4':35,'5':36,'6':37,'7':38,'8':39,'9':40,
 '0':41,'=':42,'-':43,'"':44,'+':45,'[':46,'_':47,'(':48,
 ')':49,' ':50,'?':51,'\n':52,'@':53,'#':54,'/':55,'%':56,
 '^':57,'*':58,']':59,'{':60,'}':61,'A':62,'B':63,'C':64,
 'D':65,'E':66,'F':67,'G':68,'H':69,'I':70,'J':71,'K':72,
 'L':73,'M':74,'N':75,'O':76,'P':77,'Q':78,'R':79,'S':80,
 'T':81,'U':82,'V':83,'W':84,'X':85,'Y':86,'Z':87,'<':88,
 '>':89,';':90}
#numbers to characters table
dictUp = {0:'|',1:'a',2:'b',3:'c',4:'d',5:'e',6:'f',7:'g',8:'h',
 9:'i',10:'j',11:'k',12:'l',13:'m',14:'n',15:'o',16:'p',
 17:'q',18:'r',19:'s',20:'t',21:'u',22:'v',23:'w',24:'x',
 25:'y',26:'z',27:'.',28:',',29:"'",30:'!',31:':',32:'1',
 33:'2',34:'3',35:'4',36:'5',37:'6',38:'7',39:'8',40:'9',
 41:'0',42:'=',43:'-',44:'"',45:'+',46:'[',47:'_',48:'(',
 49:')',50:' ',51:'?',52:'\n',53:'@',54:'#',55:'/',56:'%',
 57:'^',58:'*',59:']',60:'{',61:'}',62:'A',63:'B',64:'C',
 65:'D',66:'E',67:'F',68:'G',69:'H',70:'I',71:'J',72:'K',
 73:'L',74:'M',75:'N',76:'O',77:'P',78:'Q',79:'R',80:'S',
 81:'T',82:'U',83:'V',84:'W',85:'X',86:'Y',87:'Z',88:'<',
 89:'>',90:';'}
#encrypting function
def crypt(key,clearText):
 cryptText = []
 numKey = []
 keyCount = 0
 #translate the characters in the key to numbers
 for keyClearChar in key:
 keyNum = dictDown[keyClearChar]
 numKey.append(keyNum)
 for clearChar in clearText:
 #loop characters in key
 if keyCount == len(key):
 keyCount = 0 
 charNum = dictDown[clearChar]
 keyIndex = numKey[keyCount]
 cryptChar = dictUp[charNum+keyIndex%len(dictUp)]
 cryptText.append(cryptChar)
 keyCount += 1
 return ''.join(cryptText)
#inverted encrypting function
def decrypt(key,cryptText):
 clearText = []
 numKey = []
 keyCount = 0
 for keyClearChar in key:
 keyNum = dictDown[keyClearChar]
 numKey.append(keyNum)
 for cryptChar in cryptText:
 if keyCount == len(key):
 keyCount = 0
 cryptNum = dictDown[cryptChar]
 keyIndex = numKey[keyCount]
 clearChar = dictUp[cryptNum-keyIndex%len(dictUp)]
 clearText.append(clearChar)
 keyCount += 1
 return ''.join(clearText)
while True:
 choice = raw_input("Enter 'e' to encrypt,\nEnter 'd' to decrypt.\n\n")
 if choice.lower() == 'e':
 system('cls')
 key = list(raw_input("Enter an encryption key here:\n"))
 clearText = list(raw_input("\nEnter the text to be encrypted here:\n"))
 system('cls')
 cryptText = crypt(key,clearText)
 cryptDir = open("C:\Users\Yorick\Desktop\cryptText.txt",'w')
 cryptDir.write(cryptText)
 cryptDir.close()
 print("Encryption finished.\nResult saved to desktop.\nPress enter to close the program.")
 _=raw_input('')
 break
 elif choice.lower() == 'd':
 system('cls') 
 key = list(raw_input("Enter a decryption key here:\n"))
 #try opening encrypted txt file
 try:
 cryptDir = open("C:\Users\Yorick\Desktop\cryptText.txt",'r')
 cryptText = list(cryptDir.read())
 cryptDir.close()
 except IOError:
 print("No cryptText.txt found on desktop.")
 clearText = decrypt(key,cryptText)
 print(clearText)
 _=raw_input("\n\n\n\nPress enter to close.")
 break

Answer 1

This is a good place for a list comprehension:

numKey = []
for keyClearChar in key:
 keyNum = dictDown[keyClearChar]
 numKey.append(keyNum)

Can be succinctly written as

numKey = [dictDown[char] for char in key]

---

Python has the nice feature that the for loop does not care what it iterates over, as long as it is iterable. This means, you don't have to convert your keys and texts to lists, for char in "AB CD" works perfectly fine.

---

Your encrypt and decrypt function differ only by the parameter names and the sign used in the actual shift. You could combine them into one function.

from itertools import cycle
def vigenere(key, text, decrypt=False):
 sign = -1 if decrypt else 1
 numKey = [dictDown[char] for char in key]
 out = (dictUp[(dictDown[char] + sign*keyIndex) % len(dictUp)]
 for keyIndex, char in zip(cycle(numKey), text))
 return ''.join(out)

I used itertools.cycle (EDIT: itertools.zip_longest fills the shorter with a given value, we want to repeat the key, therefore cycle), which makes manually keeping track of the keyCount obsolete. I also inlined the whole calculation, though it can of course be written in a more verbose for loop. It is first build as a generator object, which is passed to join. This avoids building the list (which has the same length as the text, which is potentially big), saving memory.

EDIT: The index calculation was wrong because of missing parenthesis before the modulus (Also in your code).

You can now, if you want to, define convenience functions, as well:

def crypt(key, text):
 return vigenere(key, text)
def decrypt(key, text):
 return vigenere(key, text, True)

---

You should try to avoid polluting the namespace too much. Prefer

import os
....
os.system('cls')

(This is a bit of a preference, I did not adhere to it myself in the above function. However, it makes it obvious where a function comes from, at the price of additional characters.)

---

You should make the path to the files a constant at the beginning of the code, helping implementing command line arguments for that at a later time.

Move all your code not in a function into a main function. Then at the end of the code use:

if __name__ == "__main__":
 main(TEXT_FILE)

where TEXT_FILE is the path to the file, obviously. This way you can import this script and use its functions at a later time (e.g. if you want to use the crypt function separately in another program).

If you want to make the path the first command-line argument you can now just do:

if __name__ == "__main__":
 text_file = sys.argv[1] if len(sys.argv) > 1 else TEXT_FILE
 main(text_file)

---

The built-in module string has some nice character classes. Your dictUp is basically just this:

import string
dictUp = dict(enumerate(string.printable[:95]))

where the :95 is there to exclude the last few (tab, newline, carriage return, etc). The order might be slightly different, so your code will not be downward compatible anymore if you make this change.

string.printable == '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~ \t\n\r\x0b\x0c'

dictDown is then just the inverted dictionary of that:

def invert(d):
 return {v: k for k, v in d.iteritems()}
dictDown = invert(dictUp)

Or use itertools.count and make dictUp a simple string (dictUp[3] is not different for a list, a string or a dict, with all keys in range(95) defined).

from itertools import count
# technically not a dict anymore, but sufficient for what you use it for:
dictUp = string.printable[:95]
dictDown = dict(zip(dictUp, count()))

Note here that most python implementations cache single-letter strings, so dictUp[35] should be O(1) both for a dict and a string.

• Graipher: @TomaszJakubRup Thanks for spotting the missing closing ". Your edit was rejected because I amended the answer afterwards, but I fixed it myself. :)
• Yorick: Woah, the list comprehension is very nice. I'm going to remember that. As for the other things, I'm going to read them thoroughly and do some practice with them. Thank you for your help! :)
• Graipher: @Yorick Yes, they are nice :) Regarding the function vigenere, I just spotted a bug there, you want to use itertools.cycle, not itertools.zip_longest.
• Caridorc: Instead of a dict you could have a boolean encrypt parameter in the Vigenerè encryption, seems simpler.
• Graipher: @Caridorc you are right, a boolean is probably clearer here. Edited.

Answer 2

Here are some things that may help you improve your code.

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. I'd recommend instead using ANSI escape sequences. For example:

def cls():
 print("\x1b[2J")

Don't hardcode filenames

Hardcoding filenames is not a good idea. Better would be to either fetch the file name from the user (as with a command line argument) or to simply write the output to the console.

Write to allow for reuse

The way it's currently written, there is not a way to easily reuse the cipher in another program. One way to allow that would be to wrap the main part of the program in a function like this:

if __name__ == '__main__':

This allows you to run the program from the command line if you decide to do so, but also to use the encrypt() and decrypt() routines from other programs without modification of the file.

Use docstrings

Using docstrings would help document your program and make it simpler for others using the code (including yourself at some future date!) to understand what the various routines are doing and how.

Consider a different construction

The essential feature of both encryption and decryption is the alphabet you're using. Right now, there is no linkage between the dictUp and dictDown structures except that (you hope!) they both have all of the same letters in the same order. If you decide to continue to use dictionaries, consider constructing both structures from a single string. Alternatively, consider using a single structure for both. One mechanism would be to simply use a static string and rely on indexing.

Fix the bug

If I attempt to encrypt the word "CIPHERING" with the key "BUTTERFLY", I get the following error:

Traceback (most recent call last):
 File "encipher.py", line 86, in <module>
 cryptText = crypt(key,clearText)
 File "encipher.py", line 46, in crypt
 cryptChar = dictUp[charNum+keyIndex%len(dictUp)]
KeyError: 127