# A simple cipher that uses the sums of previous characters

I'm fairly happy with the code itself, but I'd like you to tear it apart.

def sumcipher(inpt, o=0, encrypt=True):
if not isinstance(inpt, str):
raise TypeError("Input must be a string")
if encrypt:
return ''.join([chr(((sum([ord(i) - 32 for i in inpt[j::-1]]) + (o % 96)) % 96) + 32) for j in range(len(inpt))])
else:
L = ""
for i in range(len(inpt)):
L += chr((((ord(inpt[i]) - 32) - sum([ord(j) - 32 for j in L])) % 96) + 32)
return L


I'm mostly looking for ways to make it more elegant/simple.

PS: I'm very inexperienced with asking questions.

• this is its take on the url of this question: h, |#Gu T,u!-s1s|27e!m"(z3k%y-y~IU,~Bb$p.!u&%<"&{#|%yRLfv$#|yHV&'x}-;gty39h~r.:gt}Hf"x&:bwGc"s1s&&~:V%y1o"1q-! Aug 10 '17 at 0:07

As with CRC, this doesn't offer cryptographic strength.

The o in the public API needs an informative name, perhaps "offset".

In a duck-typed language, feel free to omit checks such as:

if not isinstance(inpt, str):
raise TypeError("Input nust be a string")


since the first call to ord() will have much the same effect. Do go to the trouble of writing a docstring which describes the arguments.

Rather than an if, you might break this into two functions.

Try to show parallel structure. You chose to build up a list of strings in the 1st case, yet appended to a string in the 2nd case.

One of your modulo operations, (o % 96), seems to be redundant.

Consider naming your magic numbers, 32 and 96. Or do away with them entirely: produce binary output which you then base64 encode.

The nested loops offer quadratic performance. You have an opportunity to store sums in a temp variable if you want linear performance.