3
\$\begingroup\$

The following code:

import random

def keygen():
    l = list('abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" ')
    random.shuffle(l)
    return l

def encrypt(x):
    key = keygen() #or a static key for consistency
    y = list('abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" ')
    z = dict(zip(y, key))
    l = []
    for match in list(x):
        l.append(match.replace(match, z[match]))
    print('key: '+ ''.join(key))
    print('content: '+ ''.join(l))

def decrypt(key, x):
    key = list(key)
    y = list('abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" ')
    z = dict(zip(key, y))
    l = []
    for match in list(x):
        l.append(match.replace(match, z[match]))
    print(''.join(l))

is just a simple self made encrypter that I made for fun, and works like so:

>>> encrypt('hello world how are you')
key: ^lps1|dv)({t&hk#w;=@!6bjrx$4 \[m~"q*i+:-a73?u/5zf%gc9}yn"8_oe]`2
content: v1ttk2bk;ts2vkb2^;12rk!
>>> decrypt('^lps1|dv)({t&hk#w;=@!6bjrx$4 \[m~"q*i+:-a73?u/5zf%gc9}yn"8_oe]`2', 'v1ttk2bk;ts2vkb2^;12rk!')
hello world how are you
>>> 

This works pretty well, but I did see one time where there was a single letter that was misplaced. This could have just been due to me not copy/pasting the key in right for decoding, but I am not so sure. Basically does this code look sound? And are there any other methods of doing something like this in a more elegant way?

\$\endgroup\$
1

2 Answers 2

3
\$\begingroup\$

You should get out of the habit of printing results instead of returning them; if you return the function is reusable. Remember that you can return multiple values with:

    return ''.join(key), ''.join(l)

I would personally suggest returning a string directly from keygen.

You don't need to convert y to a list, and probably should name it better (eg. ascii). It would be better as a global constant.

encrypt and decrypt can be unified by making key optional and reversing the arguments when decrypting relative to encrypting; it's symmetric after all.

import random

ASCII = 'abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" '

def keygen():
    l = list(ASCII)
    random.shuffle(l)
    return ''.join(l)

def cipher(x, key=None, backwards=False):
    if key is None:
        key = keygen()

    z = dict(zip(key, ASCII) if backwards else zip(ASCII, key))

    l = []
    for match in list(x):
        l.append(match.replace(match, z[match]))

    return key, ''.join(l)

Since keygen is so easy to call as

encrypt(x, keygen())

I wouldn't make key have a default. This gives the simpler:

import random

ASCII = 'abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" '

def keygen():
    l = list(ASCII)
    random.shuffle(l)
    return ''.join(l)

def cipher(x, key, backwards=False):
    z = dict(zip(key, ASCII) if backwards else zip(ASCII, key))

    l = []
    for match in list(x):
        l.append(match.replace(match, z[match]))

    return key, ''.join(l)

Looking at

    l = []
    for match in list(x):
        l.append(match.replace(match, z[match]))

we have a lot of inefficiencies. You should just do

    for match in x:

and then

        l.append(z[match])

since the replace is a no-op.

This can be a list comprehension:

    l = [z[match] for match in x]

This is now just

import random

ASCII = 'abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" '

def keygen():
    l = list(ASCII)
    random.shuffle(l)
    return ''.join(l)

def cipher(x, key, backwards=False):
    z = dict(zip(key, ASCII) if backwards else zip(ASCII, key))

    l = [z[match] for match in x]
    return ''.join(l)

You can then replace some of the challenge with str.translate and str.maketrans:

import random

ASCII = 'abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" '

def keygen():
    l = list(ASCII)
    random.shuffle(l)
    return ''.join(l)

def cipher(x, key, backwards=False, base_chars=ASCII):
    if backwards:
        base_chars, key = key, base_chars

    return x.translate(str.maketrans(base_chars, key))

The bug

There is a duplicate character in ASCII. This can, and does, break things

                                                                      ↓↓
ASCII = 'abcdefghijklmnopqrstuvwxyz123456789!@#$%^&*()_-+={}[]|\:;?/~`"" '
                                                                      ↑↑

Personally, your best best is just to use string.ascii_lowercase + string.digits + string.punctuation, despite it being slightly different to the above. This gives

import random
import string

ASCII = string.ascii_lowercase + string.digits + string.punctuation

def keygen():
    l = list(ASCII)
    random.shuffle(l)
    return ''.join(l)

def cipher(x, key, backwards=False, base_chars=ASCII):
    if backwards:
        base_chars, key = key, base_chars

    return x.translate(str.maketrans(base_chars, key))

Of course, don't roll your own crypto.

\$\endgroup\$
1
  • \$\begingroup\$ kudos for spotting the repeat ". I was noticing that occasionally things were not decoding properly. \$\endgroup\$
    – riyoken
    Commented Jan 28, 2015 at 3:22
2
\$\begingroup\$

Check out the following link for how XOR Ciphers work. http://en.wikipedia.org/wiki/XOR_cipher

More specifically check the following function:

def xor_strings(s,t):
    """xor two strings together"""
    return "".join(chr(ord(a)^ord(b)) for a,b in zip(s,t))

If you're interested in more generally accepted encryption techniques check out:

https://stackoverflow.com/questions/12524994/encrypt-decrypt-using-pycrypto-aes-256

But yea it's almost always better just to use AES since it's been tried and tested.

\$\endgroup\$
1
  • \$\begingroup\$ This is not a review of the code in question, and the advice is not relevant to the algorithm. \$\endgroup\$ Commented Jan 18, 2015 at 19:49

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.