6
\$\begingroup\$

This is one of my first Python scripts and I was wondering if it meets the correct conventions. Also are there things that you would write different? I am looking for some good comments so I can start to improve my Python code from the start. (I was not supposed to use imports here)

Here's my implementation of Simplified DES:

__author__ = 'ßaron'

FIXED_IP = [2, 6, 3, 1, 4, 8, 5, 7]
FIXED_EP = [4, 1, 2, 3, 2, 3, 4, 1]
FIXED_IP_INVERSE = [4, 1, 3, 5, 7, 2, 8, 6]
FIXED_P10 = [3, 5, 2, 7, 4, 10, 1, 9, 8, 6]
FIXED_P8 = [6, 3, 7, 4, 8, 5, 10, 9]
FIXED_P4 = [2, 4, 3, 1]

S0 = [[1, 0, 3, 2],
      [3, 2, 1, 0],
      [0, 2, 1, 3],
      [3, 1, 3, 2]]

S1 = [[0, 1, 2, 3],
      [2, 0, 1, 3],
      [3, 0, 1, 0],
      [2, 1, 0, 3]]

KEY = '0111111101'


def permutate(original, fixed_key):
    new = ''
    for i in fixed_key:
        new += original[i - 1]
    return new


def left_half(bits):
    return bits[:len(bits)/2]


def right_half(bits):
    return bits[len(bits)/2:]


def shift(bits):
    rotated_left_half = left_half(bits)[1:] + left_half(bits)[0]
    rotated_right_half = right_half(bits)[1:] + right_half(bits)[0]
    return rotated_left_half + rotated_right_half


def key1():
    return permutate(shift(permutate(KEY, FIXED_P10)), FIXED_P8)


def key2():
    return permutate(shift(shift(shift(permutate(KEY, FIXED_P10)))), FIXED_P8)


def xor(bits, key):
    new = ''
    for bit, key_bit in zip(bits, key):
        new += str(((int(bit) + int(key_bit)) % 2))
    return new


def lookup_in_sbox(bits, sbox):
    row = int(bits[0] + bits[3], 2)
    col = int(bits[1] + bits[2], 2)
    return '{0:02b}'.format(sbox[row][col])


def f_k(bits, key):
    L = left_half(bits)
    R = right_half(bits)
    bits = permutate(R, FIXED_EP)
    bits = xor(bits, key)
    bits = lookup_in_sbox(left_half(bits), S0) + lookup_in_sbox(right_half(bits), S1)
    bits = permutate(bits, FIXED_P4)
    return xor(bits, L)


def encrypt(plain_text):
    bits = permutate(plain_text, FIXED_IP)
    temp = f_k(bits, key1())
    bits = right_half(bits) + temp
    bits = f_k(bits, key2())
    print permutate(bits + temp, FIXED_IP_INVERSE)


def decrypt(cipher_text):
    bits = permutate(cipher_text, FIXED_IP)
    temp = f_k(bits, key2())
    bits = right_half(bits) + temp
    bits = f_k(bits, key1())
    print permutate(bits + temp, FIXED_IP_INVERSE)


encrypt('11101010')
decrypt('10100010')
\$\endgroup\$
  • \$\begingroup\$ Is doing this object-oriented allowed? \$\endgroup\$ – Mast Oct 19 '15 at 19:24
  • \$\begingroup\$ Yes, it is allowed. \$\endgroup\$ – ßaron Oct 19 '15 at 19:25
  • \$\begingroup\$ What is "Simplified DES"? Is it the algorithm described here? \$\endgroup\$ – Gareth Rees Oct 19 '15 at 19:25
  • \$\begingroup\$ Hello @Gareth Rees, that is indeed the algorithm, i should have given the link.. \$\endgroup\$ – ßaron Oct 19 '15 at 19:27
  • 1
    \$\begingroup\$ @ßaron It affects the speed here is the main reason I asked, the more recent you're using the less of a difference Caridorc's answer makes. Also to reply to people in comments make sure you @ them as people don't always got notified otherwise! \$\endgroup\$ – SuperBiasedMan Oct 20 '15 at 12:47
3
\$\begingroup\$

If you're using constants that are collections you'd be better off making them tuples, not lists. Trying to modify a tuple will raise an error as they're immutable, and using the tuple syntax makes it extra clear that these are unchanging constants.

FIXED_IP = (2, 6, 3, 1, 4, 8, 5, 7)
FIXED_EP = (4, 1, 2, 3, 2, 3, 4, 1)

As Caridorc said, using str.join is faster (depending on your version). It would also allow you to make permutate just one line.

def permutate(original, fixed_key):
    return ''.join(original[i - 1] for i in fixed_key)

It would also be a bit faster to pre-convert all your values in bits and key to integers. You can use map to apply a function to every member of a list, and it's faster than doing it in a list. You could do this when creating the zip object.

def xor(bits, key):
    new = ''
    for bit, key_bit in zip(map(int, bits), map(int, key)):
        new += str(((bit + key_bit) % 2))
    return new

Of course if you wanted this could also be made into a str.join, albeit a long one:

def xor(bits, key):
    return ''.join(str(((bit + key_bit) % 2)) for bit, key_bit in
                   zip(map(int, bits), map(int, key)))

There's a lot of functions here but not a lot of documentation. Explaining what individual functions do would make your code a lot easier to read. And I suspect you have some unnecessary duplicate functions, where you could pass a parameter instead of defining a whole new one. But it's hard to know when I don't entirely understand the functions you have here.

\$\endgroup\$
1
\$\begingroup\$

It is well written overall, but remember that string += thing is very slow when used in loops and you should join a generator expression for efficiency.

\$\endgroup\$
  • \$\begingroup\$ In modern versions of CPython repeated string concatenation is usually only a little slower than join, so this is now only a worry if you want to be portable to other versions of Python like PyPy. \$\endgroup\$ – Gareth Rees Oct 19 '15 at 21:09
  • \$\begingroup\$ @GarethRees just tested, in my Cython 3.4 joining is actually slower (by 20%) but in pypy += is so slow as to be totally unusable (more than 100x the time of join and still running) \$\endgroup\$ – Caridorc Oct 19 '15 at 21:31
  • \$\begingroup\$ That's right, so I think that when you give the advice not to use string concatenation you should mention that the advice might not make any difference in CPython, but is still a good idea if you are concerned about portability. \$\endgroup\$ – Gareth Rees Oct 20 '15 at 11:31

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.