2
\$\begingroup\$

My first piece of code encrypts text by moving each letter in the string 5 letters across in the alphabet.

encrypt = input('Enter text to encrypt : ')
encrypt = encrypt.lower().replace(" ", "")
for i in encrypt:
    print(chr(ord(i) + 5))
decrypt = input('Enter text to decrypt : ')
decrypt = decrypt.lower().replace(" ", "")
for i in decrypt:
    print(chr(ord(i) - 5))

The output of this code if I enter 'hello' would be 'mjqqt'.

To decrypt, I do the reverse, I move each letter 5 letters down the alphabet.


Below is my second piece of code that encrypts text against a code word. For example...

If I had 'abc' for the codeword and 'bcd' for the text to encrypt. Then: (a + b = 1 + 2) and this is repeated for each letter in the two words. The output of having my codeword as 'abc' and the word to encrypt and 'bcd' would be 'ceg'.

codeword = input('Enter codeword : ')
codeword = codeword.replace(" ", "")  

encrypt = input('Enter text to encrypt : ')
encrypt = encrypt.replace(" ", "")

j = 0
for i in codeword:
    print(chr(ord(encrypt[j])+ ord(codeword[j])-96))
    j+=1
\$\endgroup\$
  • \$\begingroup\$ If I have functions for encryption and decryption, I generally expect that encrypt(decrypt(s)) == decrypt(encrypt(s)) == s for all strings s, modulo padding to a boundary. Because you’re throwing away spaces and casting the string to lowercase, that isn‘t true here. \$\endgroup\$ – alexwlchan Feb 27 '16 at 12:12
  • \$\begingroup\$ looks like you have two sets of code to review, I recommend a separate questions 1 for each set of code. ask for one set of code and then clean up that one and the other set of code before you post that second piece of coding marvel. \$\endgroup\$ – Malachi Feb 27 '16 at 15:16
  • 5
    \$\begingroup\$ Please do not add, remove, or edit code in a question after you've received an answer. The site policy is explained in What to do when someone answers. If you disagree with the rollback you'll have to contact a moderator or use the contact us section. Keep in mind removing code is not something done lightly. \$\endgroup\$ – Mast Mar 1 '16 at 7:43
  • \$\begingroup\$ well how do I delete this post??? It wont let me because It has answers \$\endgroup\$ – D.C Mar 1 '16 at 18:37
5
\$\begingroup\$

Part 1, Caesar cipher

Instead of printing one character at a time (inefficient), create a string using the str.join method.

''.join(chr(ord(char) + 5) for char in text)

The variable name i is associated with integer indexes and is misleading to use for a character.

Separate the encryption/decryption into a function

Rather than forcing printing text to screen, a function returning a string can be used in any other context as well. It also lets us use the similarity between encryption and decryption to reduce repetition in the code.

def caesar_shift(text, places=5):
    text = text.lower().replace(' ', '')
    return ''.join(chr(ord(char) + places) for char in text)

The number of places to shift is a parameter instead of hardcoded, with a default of 5 to work the same as your example when the argument is omitted. A value of -5 then decrypts the resulting string.

Handle out-of-range characters

The technique appears to be meant to only work on (English) letters. To avoid unintended results on different inputs, one option is to leave other characters unmodified.

Also, "wrap around" instead of substituting for characters outside of the alphabet, by using the modulus operator %.

from string import ascii_lowercase

def caesar_shift(text, places=5):
    def substitute(char):
        if char in ascii_lowercase:
            char_num = ord(char) - 97
            char = chr((char_num + places) % 26 + 97)
        return char
    text = text.lower().replace(' ', '')
    return ''.join(substitute(char) for char in text)

With the character substitution logic now being too long to fit comfortably in one line, I chose to separate it into a nested function.

Part 2, Vigenère cipher

Loop variables

Your loop here is somewhat confusing, in that the actual loop variable i goes unused while an additional variable j is used as the index. It could be written as follows to avoid that particular issue (using min in case the lengths are different).

for i in range(min(len(text), len(key))):
    print(chr(ord(text[i]) + ord(key[i]) - 96))

An even better, more idiomatic way to loop over multiple sequences (strings, lists etc.) is with the built-in zip function, which creates a tuple of elements for every loop iteration.

for text_char, key_char in zip(text, key):
    print(chr(ord(text_char) + ord(key_char) - 96))

Write as a function

Let's also apply the advice from the previous part and write it as a function.

def vigenere(text, key):
    return ''.join(caesar_shift(tc, ord(kc)-96) for tc, kc in zip(text, key))

Notice how it can be implemented using the caesar_shift function from above.

Handle different text and key lengths

What if the text to be encrypted or decrypted and the key are of different lengths? The convention is to repeat the key phrase.

While it wouldn't be too hard to write code for this from the beginning, it's often a good idea to look for something useful in the standard library. In this case, the itertools.cycle function turns out to be convenient.

from itertools import cycle

def vigenere(text, key):
    key = cycle(key)
    return ''.join(caesar_shift(tc, ord(kc)-96) for tc, kc in zip(text, key))

I've left out the input and output parts from this answer.

\$\endgroup\$

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.