5
\$\begingroup\$

This is my first Python project I have written. It is a simple Encrypter/Decrypter.
It has 4 commands:

  • encrypt (tekst) - encrypts text and gives you a decryption key.
  • key (key) - set the decryption key for decryption.
  • decrypt (tekst) - decrypts tekst.
  • exit - end program.

I am looking for some feedback on this. Because its my first time I was wondering if you guys could give me some tips on what I did wrong/how I could make the code better. It works like intended at the moment but I don't now if there are any bugs.
Hope you guys can give me some tips on how to make this code better!

import random

print("""  _____                                   _            
 |  __ \                                 | |           
 | |  | | ___ _ __   ___ _ __ _   _ _ __ | |_ ___ _ __ 
 | |  | |/ _ \ '_ \ / __| '__| | | | '_ \| __/ _ \ '__|
 | |__| |  __/ | | | (__| |  | |_| | |_) | ||  __/ |   
 |_____/ \___|_| |_|\___|_|   \__, | .__/ \__\___|_|   
                               __/ | |                 
                              |___/|_|                 """)
woord = ''
exit = False
key = 0
while not exit:
    arguments = ''
    woord = input('> ')
    command = woord.split(' ', 1)[0]
    letters = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f',
               'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
               'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L',
               'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '!', '"',
               '#', '$', '%', '&', "'", '(', ')', '*', '+', ',', '-', '.', '/', ':', ';', '<',
               '=', '>', '?', '@', '[', '\\', ']', '^', '_', '`', '{', '|', '}', '~', ' ']
    if woord != command:
        arguments = woord.split(' ', 1)[1]
    if woord == 'exit':
        exit = True
        print('Goodbeye!')
    elif command == 'encrypt':
        if arguments != '':
            tekst = []
            tekstpos = []
            verplaatsing = random.randrange(1, 10)
            x = 0
            y = 0
            versleuteld = []
            for line in arguments:
                tekst.extend(line)
            while x < len(tekst):
                position = letters.index(tekst[x])
                if position < len(letters) - verplaatsing:
                    position += verplaatsing
                elif position > len(letters) - verplaatsing:
                    position -= verplaatsing
                tekstpos.append(position)
                x += 1
            while y < len(tekstpos):
                cijfer = tekstpos[y]
                versleuteld.append(letters[cijfer])
                y += 1
            print('Encrypted tekst: ' + ''.join(versleuteld))
            print('Encryption key: ' + str(verplaatsing))
        else:
            print('The crypt command requires an argument. \nUse like this: crypt (tekst)')
    elif command == 'key':
        if arguments != '':
            key = int(arguments)
            print('Decryption key changed to: ' + str(key))
        else:
            print('Decryption key: ' + str(key))
    elif command == 'decrypt':
        if arguments != '' and key != 0:
            tekst = []
            tekstpos = []
            x = 0
            y = 0
            ontsleuteld = []
            for line in arguments:
                tekst.extend(line)
            while x < len(tekst):
                position = letters.index(tekst[x])
                if position < len(letters) - key - 1:
                    position -= key
                elif position >= len(letters) - key - 1:
                    position += key
                tekstpos.append(position)
                x+= 1
            while y < len(tekstpos):
                cijfer = tekstpos[y]
                ontsleuteld.append(letters[cijfer])
                y += 1
            print('Decrypted tekst: ' + ''.join(ontsleuteld))
        elif key == 0:
            print('Decrpyption key not defined')
        else:
            print('The encrypt command requires an argument. \nUse like this: encrypt (tekst)')
\$\endgroup\$
7
\$\begingroup\$

The Algorithm

When using algorithms, it is helpful to name things accordingly so others can more easily understand what is going on. Even a novel algorithm can be somewhat categorized.

Your encryption algorithm is a substitution cipher, more specifically a monoalphabetic cipher. This means that for any given character m there is a new character c which substitutes it. A famous example of a monoalphabetic cipher is the Caesar cipher which creates the substitution alphabet by shifting the original alphabet.

Looking through your algorithm, I would categorize your algorithm as a Caesar cipher with a slight variation caused by this if:

if position < len(letters) - verplaatsing:
    position += verplaatsing
elif position > len(letters) - verplaatsing:
    position -= verplaatsing

Depending on the character you shift to right or the left. But this causes a problem, the small portion that is shifted to the left overlaps with the rest and causes different m to be substituted with the same c which means your cipher does not work. For example when encrypting #$%&'()*+,-./:;=>?@[\]^_`{|\~ with key 7 we get *+,-./:;<=>?@[\^_`{|}~ _?@[}], notice the duplicate ?@[ part.

As a side note, there is a bug in your decryption, which I found while testing this edge case. The condition in the if should be position < len(letters) + key - 1 with a + in front of the key because you are shifting in the other direction.

It seems like you wanted to implement a Caesar cipher but were unsure how to handle the case where you are shifting beyond your letter array. For a Caesar cipher you can simply use modulo:

index = letters.index(message)
newindex = (index + key) % len(letters)
cipher = letters[newindex]

For values where index + key < len(letters) nothing changes, but for values greater you wrap around to the beginning. This also simplyfies your encryption and has no out of range error.

In general do not roll you own crypto. When experimenting go nuts with your ideas but do not use it anywhere.

Programming Stuff

You do not need a while loop for iterating over a list. The idiomatic way is:

for cijfer in tekstpos:
    ontsleuteld.append(letters[cijfer])

When you need a counter the idiomatic way would be:

for x in range(len(tekst)):
    position = letters.index(tekst[x])
    if position < len(letters) - key - 1:
        position -= key
    elif position >= len(letters) - key - 1:
        position += key
    tekstpos.append(position)

Where range(n) creates a list of the form [0, 1, 2, ..., n-1]. These changes help limit your variables to a narrower scope so that they do not clutter the code.

\$\endgroup\$
2
  • \$\begingroup\$ Also really helpfull. Deffinatly gone try to implement the Caeser cipher to make it work proparly \$\endgroup\$ – Barkork Mar 11 '18 at 9:30
  • \$\begingroup\$ If you are interested in ciphers, you can look into the en.wikipedia.org/wiki/Vigen%C3%A8re_cipher which is a simple extension to the Caesar cipher. \$\endgroup\$ – henje Mar 11 '18 at 21:45
7
\$\begingroup\$

There's a lot to be said about the algorithm you used, but I'll keep it to general advice.

  1. letters is already defined in the string module as string.printable.

  2. It is strongly discouraged to use non-English vocabulary in code (including variable names, comments, and docstrings). You should always assume people from all over the world are going to collaborate. In this particular example, it's okay, since the script is for personal use, but keep this in mind for larger (open-source) projects.

  3. The code isn't reusable. Because you put everything into the top-level namespace, as soon as I try to import it, I get a bunch of output and an input prompt. We can solve this by separating the code into dedicated functions, in pseudocode:

    function encrypt(text, key):
        # encryption procedure
        return encryptedText;
    
    function decrypt(text, key):
        # decryption procedure
        return decryptedText;
    

    This eliminates the need for a 'key' procedure, because the key is passed as an argument to the functions. In case you haven't covered functions yet, be sure to return here later and improve your code with functions.

  4. This may not be your biggest concern right now, but random.randrange is not cryptographically secure, since it uses the Mersenne Twister as random number generator. The Python 3.6 release introduced secrets to the standard library. If you want to stick with random, try random.SystemRandom.

  5. You assume that user input is well-formed, but this doesn't necessarily have to be the case. For example, key = int(arguments) may break if arguments is not numerical. By the EAFP (Easier to Ask for Forgiveness than for Permission) principle, you should wrap it in a try: ... except: ... block:

    try:
        key = int(arguments)
    except ValueError:
        print("Key must be numerical!")
    

    It can be a pain to do this every time you want an integer, so you may wrap it in a function for future use:

    def get_int(prompt):
        while True:
            response = input(prompt)
            try:
                return int(response)
            except ValueError:
                print("Response must be an integer.")
    
\$\endgroup\$
0

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.