Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The encryption script:

import random
splitArray = []

def takeInput():

    rawText = raw_input("Type something to be encrypted: \n")
    password = raw_input("Please type a numerical password: \n")
    encrypt(rawText, int(password))

def encrypt(rawText, password):
    for c in rawText:
        divide(c, password)


def divide(charToDivide, password):
    asciiValue = ord(charToDivide)
    a = 0
    b = 0
    c = 0
    passa = 0
    passb = 0
    passc = 0
    a = random.randrange(a, asciiValue)
    b = random.randrange(b, asciiValue - a)
    c = asciiValue - (a+b)
    passa = random.randrange(passa, password)
    passb = random.randrange(passb, password-passa)
    passc = password - (passa+passb)
    if isinstance(password, str):   
        print "Please enter a number"
        takeInput()
    else:
        a += passa  
        b += passb
        c += passc      
    splitArray.append(str(a))
    splitArray.append(str(b))
    splitArray.append(str(c))

takeInput()
f = open("myEncryptorTest", 'w')
arrayDelimiterString = "."
encryptedString = arrayDelimiterString.join(splitArray)
encryptedString = "." + encryptedString
f.write(encryptedString)
f.close

Decryption:

#XECryption decryption program

#Each character is a set of three numbers delimited by dots

#Decryption works by adding these three numbers together, subtracting the ASCII for a space and using that number to decypher 
#the rest of the array.

#User is prompted to see if the message makes sense

f = open('myEncryptorTest')
encryptedString = f.read()
f.close()

#separate sets of three numbers into an array
def sort():
    sortedCharArray = []
    charBuffer = "" 
    for c in encryptedString:
        if c == '.' and charBuffer != "":
            sortedCharArray.append(charBuffer)
            charBuffer = ""
        elif c != '.':
            charBuffer += c
    #if the buffer is not empty (e.g. last number), put it on the end
    if charBuffer != "":
        sortedCharArray.append(charBuffer)
        charBuffer = ""
    crack(sortedCharArray)

#add sets of three numbers together and insert into an array decryption
def crack(charArray):
    charBuffer = 0
    arrayCount = 1
    decypheredArray = []
    for c in charArray:
        if arrayCount % 3 == 0:
            arrayCount = arrayCount + 1
            charBuffer = charBuffer + int(c)
            decypheredArray.append(charBuffer)
            charBuffer = 0
        else:
            arrayCount = arrayCount + 1
            charBuffer = charBuffer + int(c)
    decypher(decypheredArray)

#subtract ASCII value of a space, use this subtracted value as a temporary password
def decypher(decypheredArray):
    space = 32
    subtractedValue = 0
    arrayBuffer = []
    try:
        for c in decypheredArray:
            subtractedValue = c - space
            for c in decypheredArray:
                asciicharacter = c - subtractedValue
                arrayBuffer.append(asciicharacter)
            answerFromCheck = check(arrayBuffer)
            if answerFromCheck == "y":
                #print value of password if user states correct decryption
                print "Password: "
                print subtractedValue   
                raise StopIteration()
            else:
                arrayBuffer = []
    except StopIteration:
        pass

#does the temporary password above produce an output that makes sense?
def check(arrayBuffer):
    decypheredText = ""
    stringArray = []
    try:
        for c in arrayBuffer:
            try:
                stringArray.append(chr(c))
            except ValueError:
                pass    
        print decypheredText.join(stringArray)
        inputAnswer = raw_input("Is this correct?")
        if inputAnswer == "y":
            return inputAnswer
        else:
            stringArray = []
            return inputAnswer
    except StopIteration:
        return 
sort()
f.close()

As I say, I'm looking for advice on how to improve my code and writing code in general. I'm aware that my code is probably an affront against programmers everywhere but I want to improve. These two scripts are for the hackthissite.org Realistic 6 mission. I won't be using them for encrypting anything of great importance.

share|improve this question

migrated from stackoverflow.com Sep 9 '11 at 16:56

This question came from our site for professional and enthusiast programmers.

    
Unless you have a specific and very good reason for writing your own encryption algorithm, you most likely shouldn't do it. diovo.com/2009/02/wrote-your-own-encryption-algorithm-duh –  Jeff Welling Sep 9 '11 at 19:11
    
I wrote this for the hackthissite.org Realistic 6 mission. –  user331296 Sep 9 '11 at 20:21
    
codereview.stackexchange.com/… –  Tshepang Sep 10 '11 at 5:12

1 Answer 1

import random
splitArray = []

It is best not to store data in global variables. Instead, you should have functions take all the input they need and return the result

def takeInput():

    rawText = raw_input("Type something to be encrypted: \n")

The python style guide recommends lowercase_with_underscores for local variable names

    password = raw_input("Please type a numerical password: \n")
    encrypt(rawText, int(password))

If the user enters something not a number, you'll get an exception. Its best to catch the exception and ask the user to try again

def encrypt(rawText, password):
    for c in rawText:
        divide(c, password)
        print c     

Why are you printing c? If its just debug output you should remove when you've finished debugging.

def divide(charToDivide, password):
    asciiValue = ord(charToDivide)
    a = 0
    b = 0
    c = 0

Consider storing these as a tuple

    passa = 0
    passb = 0
    passc = 0

And here as well

    a = random.randrange(a, asciiValue)

Ok, why did you assign zero to a earlier rather then just putting 0 as the argument here

    b = random.randrange(b, asciiValue - a)

Same here

    c = asciiValue - (a+b)
    passa = random.randrange(passa, password)
    passb = random.randrange(passb, password-passa)
    passc = password - (passa+passb)

And again here, the earlier assignments seemed to have no purpose. Additionally, the same logic was applied to both asciiValue and password. Make a function that produces the a,b,c from the input and call it for each of those values instead.

    if isinstance(password, str):   
        print "Please enter a number"
        takeInput()

Given your code how could password possibly be a string? If it were you would have already gotten an exception. You also shouldn't really mix function that perform logic and ones that take input. else: a += passa
b += passb c += passc
splitArray.append(str(a)) splitArray.append(str(b)) splitArray.append(str(c))

If you take my advice and put a,b,c in a tuple, then you could done this in one line.

takeInput()
f = open("myEncryptorTest", 'w')
arrayDelimiterString = "."
encryptedString = arrayDelimiterString.join(splitArray)

There isn't a whole lot of point in assigning a variable in one line and then using it only on the next.

encryptedString = "." + encryptedString

Instead of that:

encryptedString = ''.join('.' + data for data in splitArray)


f.write(encryptedString)
f.close

You are missing the (), so the file won't actually be closed.

#XECryption decryption program

#Each character is a set of three numbers delimited by dots

#Decryption works by adding these three numbers together, subtracting the ASCII for a space and using that number to decypher 
#the rest of the array.

#User is prompted to see if the message makes sense

Python has a feature called docstrings. If the first thing in a module or function is a string, it it considered the documentation for that object. You would do something like

"""
XEcryption decryption program

yada yada yada
"""

Rather then all those comments

f = open('myEncryptorTest')

I recommend against single-letter variable names

encryptedString = f.read()
f.close()


#separate sets of three numbers into an array

This comment should be a docstring in the function like I described. Also, the function doesn't do anything with sets of three numbers.

def sort():
    sortedCharArray = []
    charBuffer = "" 
    for c in encryptedString:
        if c == '.' and charBuffer != "":
            sortedCharArray.append(charBuffer)
            charBuffer = ""
        elif c != '.':
            charBuffer += c
    #if the buffer is not empty (e.g. last number), put it on the end
    if charBuffer != "":
        sortedCharArray.append(charBuffer)
        charBuffer = ""

This entire function can be replaced by sortedCharArray = encryptedString.split('.') Also, the function doesn't do any sorting.

    crack(sortedCharArray)

#add sets of three numbers together and insert into an array decryption
def crack(charArray):
    charBuffer = 0
    arrayCount = 1
    decypheredArray = []
    for c in charArray:
        if arrayCount % 3 == 0:

Don't iterate over what you have, iterate over what you want. Do something like:

    for index in xrange(0, len(charArray), 3):
        triplet = charArray[index:index+3]

That way triplet will have the three elements and you can operate on those without having to keep track of charBuffer or arrayCount

            arrayCount = arrayCount + 1
            charBuffer = charBuffer + int(c)
            decypheredArray.append(charBuffer)
            charBuffer = 0
        else:
            arrayCount = arrayCount + 1
            charBuffer = charBuffer + int(c)
    decypher(decypheredArray)

Rather then calling the next function in a chain like this. I suggest that you return at the end of each function and then have a master function pass that onto the next function. That way the master function will have a list of all of the steps in the algorithm. But bonus points for actually splitting it up into multiple functions

#subtract ASCII value of a space, use this subtracted value as a temporary password
def decypher(decypheredArray):
    space = 32

Why don't you use space = ord(' ')

    subtractedValue = 0
    arrayBuffer = []
    try:
        for c in decypheredArray:
            subtractedValue = c - space
            for c in decypheredArray:

Careful! your inner loop has the same variable as the outer loop asciicharacter = c - subtractedValue arrayBuffer.append(asciicharacter) answerFromCheck = check(arrayBuffer) if answerFromCheck == "y":

Use the python values True and False, not strings.

                #print value of password if user states correct decryption
                print "Password: "
                print subtractedValue   
                raise StopIteration()

Use break or return. You almost never need to raise StopIteration else: arrayBuffer = []

Rather then clearing the arrayBuffer here, create a new arrayBuffer except StopIteration: pass See this is problematic because something else could raise a StopIteration and you might accidently catch it.

#does the temporary password above produce an output that makes sense?
def check(arrayBuffer):
    decypheredText = ""
    stringArray = []
    try:
        for c in arrayBuffer:
            try:
                stringArray.append(chr(c))
            except ValueError:
                pass

If you get a ValueError, its because the character in question wasn't ascii. Shouldn't you just assume that the password was incorrect in that case?

Try stringArray = map(chr, arrayBuffer) instead of that loop.

        print decypheredText.join(stringArray)
        inputAnswer = raw_input("Is this correct?")

You are mixing input and logic. I recommend reworking your code so they are seperate.

        if inputAnswer == "y":
            return inputAnswer
        else:
            stringArray = []

This has no effect outside this function, why are you doing it?

            return inputAnswer
    except StopIteration:
        return 
sort()
f.close()
share|improve this answer
    
This is exactly what I was after. Thanks for taking the time to reply. –  user331296 Sep 11 '11 at 7:18
    
The password can be an ASCII character now, the encryption script basically adds the ASCII values of the characters together and uses that as a password. I'll go over the rest of the code with your advice. –  user331296 Sep 11 '11 at 7:28
1  
@user331296 if you agree the answer please tag it as accepted in front of answer –  Xavier Combelle Sep 12 '11 at 6:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.