3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ 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-! \$\endgroup\$ Aug 10 '17 at 0:07
2
\$\begingroup\$

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.

\$\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.