-3
\$\begingroup\$

Here is a code I'm working on that alters a individual letters.Each letter is pumped up x amount of letters. This alteration is dependent on the length of the word.

So for instance "hi" becomes "jk", "there" becomes "ymjwj", and thus "hi there" is "jk ymjwj".

I want to this code to not encrypt put to also decrypt messages. So the user could translate, "jk ymjwj" back to hi there using this formula.

Right now when the code is ran in terminal it produces indentation errors. I am still a python newbie so I'm uncertain how to fix these specific issues.`

#Letter altering code
#This is to use string.ascii_lowercase which is a lowercase alphabet 
import string
alphabet = string.ascii_lowercase
textIn= ""
trans = raw_input("Press 'e' to encrypt. \n Press 'd' to decrypt. \n press 'q' to quit.")
if trans.lower() == 'e':
    while textIn.lower() != 'q':
        textIn = raw_input("Type a sentence to be translated ('q' to quit).\n").lower()
        textOut = ""
        if textIn == 'q':
            pass
        else:
            for word in textIn.split():
                newWord = ""
                for char in word:
                    if char in alphabet:
                        pos = alphabet.index(char)
                        newPos = (pos + len(word))%26 
                        newChar = alphabet[newPos]
                        newWord += newChar
                    else:
                        newWord += char
                textOut += newWord + " " 
        print(textOut)  
elif trans.lower() == 'd':
    while textIn.lower() != 'q':
        textIn = raw_input("Type a sentence to be translated ('q' to quit).\n").lower()
        textOut = ""
        if textIn == 'q':
            pass
        else:
            for word in textIn.split():
                newWord = ""
                for char in word:
                    if char in alphabet:
                        pos = alphabet.index(char)
                        newPos = (pos - len(word))%26 
                        newChar = alphabet[newPos]
                        newWord += newChar
                    else:
                        newWord += char
                textOut += newWord + " " 
        print(textOut)
elif trans.lower() == 'q':
    pass
else:
    print "Command not found. Please try again."
    trans = raw_input("Press 'e' to encrypt. \n Press 'd' to decrypt. \n press 'q' to quit.")

It would also be nice to incorporate the ablitly import a txt document to be coded or decoded. For now I want to get this working.

\$\endgroup\$
  • 2
    \$\begingroup\$ Indentation is very important in Python. If you have indentation errors, your code is broken and considered off-topic for Code Review. \$\endgroup\$ – Mast Sep 22 '15 at 16:27
  • \$\begingroup\$ Your code works fine for me after changing <SPACE>print(textOut) to print(textOut), if you fix that trivial error, we may review your code \$\endgroup\$ – Caridorc Sep 22 '15 at 16:30
  • \$\begingroup\$ It seems that you are mixing tabs and spaces for indentation. Better use only spaces for Python code (e.g. by setting up your text editor to insert four spaces when you hit the tab key). \$\endgroup\$ – mkrieger1 Sep 22 '15 at 16:57
  • 1
    \$\begingroup\$ Close voters: I have made the (frankly trivial) fix to get the code to run, so that I can review it without risk of the question being closed. I’m writing my answer now, so no more close votes please? :-) \$\endgroup\$ – alexwlchan Sep 22 '15 at 17:02
1
\$\begingroup\$

Here are some high-level comments:

  • As stated in the comments, indentation matters in Python, and as it stands your code wouldn’t compile.

  • There isn’t much in the way of documentation or comments. Although this seems like a fairly trivial piece of code, it’s a good habit to get into – it makes code substantially easier to read, review and maintain.

    The one comment – about the purpose of importing string – is not particularly helpful, because the only use of string comes on the very next line.

  • There’s a lot of repeated code, and nothing here can be reused. The encrypt and decrypt options differ by only one character.

    This is just begging to be broken into smaller functions.

  • You should read PEP 8, the Python style guide. Among other things, you should look at line length and variable names.

Having skimmed the code, I’m going to dive in and try it. Let’s see what happens:

  • If I make a mistake when entering my initial option, the script simply exits silently. That’s not a very good experience:

    $ python caesarlen.py
    Press 'e' to encrypt.
     Press 'd' to decrypt.
     press 'q' to quit.x
    Command not found. Please try again.
    Press 'e' to encrypt.
     Press 'd' to decrypt.
     press 'q' to quit.e
    

    Also, it would be nice if there was a newline and maybe a prompt character (say >) to help distinguish my input from the program’s output.

  • Once I’ve chosen an option (encrypt or decrypt), I have to restart the script to change my mind. That’s not a very good flow.

    A good flow when testing encryption code is to encrypt a string, then decrypt the output to check you get the original string. Having to quit and restart the script is mildly annoying; it would be better it I could choose the action on every run.

  • It would be good if there were some newlines sprinkled through the output to break it up a bit. Right now it’s all tight and hunched together:

    $ python caesarlen.py
    Press 'e' to encrypt.
     Press 'd' to decrypt.
     press 'q' to quit.d
    Type a sentence to be translated ('q' to quit).
    xlmw ku b amvbmvkm
    this is a sentence
    Type a sentence to be translated ('q' to quit).
    this is a sentence
    pdeo gq z kwflwfuw
    

    How about this:

    $ python caesarlen.py
    Press 'e' to encrypt.
    Press 'd' to decrypt.
    press 'q' to quit.
    > d
    
    Type a sentence to be translated ('q' to quit).
    > xlmw ku b amvbmvkm
    this is a sentence
    
    Type a sentence to be translated ('q' to quit).
    > this is a sentence
    pdeo gq z kwflwfuw
    

  • Overall, the script seems pretty robust to user input. I’m unable to induce a traceback, exception or generally make it misbehave. 👍

Back to the code. Here’s some more detailed feedback:

  • In quite a few places, you get the user’s raw_input and convert to lowercase. Wrap that in a function?

  • Your encryption and decryption functions use this pattern:

    text_in = ""
    while text_in.lower() != 'q':
        text_in = get_string_from_user()
        if text_in == 'q':
            pass
    

    Rather than using a pass here, it would be better to use a break: that will explicitly break you out of the while loop without doing any more work. It also means you can simplify the while loop as follows, and save yourself from initialising the variable:

    while True:
        text_in = get_string_from_user()
        if text_in == 'q':
            break
    

    One other nitpick on the original while loop: you’re coercing all user input to lowercase, so checking text_in.lower() != 'q' is equivalent to checking text_in != 'q'. That simplification can be applied throughout.

  • Your variable names could be better. Phrases like textIn and textOut are a very dry description; you should explain what these variables represent. Better alternatives would be something like plaintext and ciphertext.

  • Be consistent about using print as a statement (line 48) vs a function (lines 25 and 44). I would suggest using it as a function, because it will make life easier when you move to Python 3.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I don't think I am supposed to thank you with these comments, but I want to do that anyhow. I appreciate your patience and thoroughness. You have given me plenty to work on! \$\endgroup\$ – Collin Stump Sep 23 '15 at 18:46

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