7
\$\begingroup\$

What do you think about this code? I'm learning Python and I made this tool to code a plain text or decode it!

inp = input('Input the text you want to code:\n')
inp = inp.lower()

key = int(input('Input the key you want to use from 2 to 25:\n'))
def rot13(input,key): #Function to code a text with caeser chyper.
    if key > 25:
        key = 25
    elif key < 2:
        key = 2
    finaltext = ''
    for letter in input:
        if letter.isalpha():
            num = ord(letter)
            if (num + key) > 122: #If the final number is greater than 122..
                x = (num + key) - 122
                finaltext += chr(x + ord('a') - 1)
            elif((num + key <= 122)):
                finaltext += chr(num + key)
        else:
            finaltext += letter
    print(finaltext)


rot13(inp,key)
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I think you should add the tag reinvent-the-wheel otherwise you'll get responses telling you that it's already been done. \$\endgroup\$
    – ChatterOne
    Mar 4, 2017 at 18:07
  • \$\begingroup\$ @ChatterOne It's only reinventing-the-wheel if there is a standard Python implementation of the Caesar Cipher, and the poster is deliberately avoiding it. \$\endgroup\$ Mar 4, 2017 at 18:42

3 Answers 3

9
\$\begingroup\$

I'd say the code is not bad, just a few style issues and a couple of practical things.

  • You don't need two lines to lowercase the input string, you can do directly

    inp = input('Input the text you want to code:\n').lower()

  • You're not checking user input, if the second input is not an integer, int() will generate a ValueError. You should put a try/catch around that.

  • Your input implies that numbers lower than 1 and higher than 25 are not acceptable, while actually you're accepting them and doing something different than what you told the user. Either you generate an error and refuse the input, or you say that the input will be modified accordingly. Either way the user should know what's going on.

  • You have quite a few hard-coded values, consider using constants .

  • The comment # If the final number is greater than 122.. is useless, the code is clear at that point.

  • The comment # Function to code a text with caeser chyper. should actually be a docstring . You should also specify what you expect as input and what kind of output you return.

  • I would rename the variable input to input_string , just so it's not the same name of the input() function.

  • elif((num + key <= 122)): is actually just else:

  • You're printing inside of the function. I'd return the value and let the caller decide what to do with it.

  • You may want to consider putting if __name__ == "__main__": in your code and call your function from there.

  • This is a simple function, so no need to dig too deep but if you want, you may have a look here about string concatenation .

\$\endgroup\$
1
3
\$\begingroup\$

Function name

rot13 is a specific name given to a general function. According to Wikipedia ROT13 is:

a simple letter substitution cipher that replaces a letter with the letter 13 letters after it in the alphabet. ROT13 is a special case of the Caesar cipher, developed in ancient Rome.

So this name denotes that the function rotates a string only by 13, which it doesn't. So you can give it a more general name like rotated_word.

Documentation

You've documented your funtion the wrong way using comments instead of a docstring (read it, it's another useful link). Docstrings are better than comments in documenting functions, as you can view a function's docstring using the built-in function help, to know how to use the function.

Remove # If the final number is greater than 122.. it's not needed. You suppose that whoever reads your code knows Python, so you don't need to explain everything to them.

Return value

Your function prints the result instead of returning it, which gets the job done but makes the function less reusable. Suppose that you want to read a textfile (using Python), encrypt it using Caesar cipher, and then write the ciphertext to a new textfile. Or that you have a list of strings and want to make a new list with these strings encrypted. What will you do? Sure you can make other functions to get this done, but if you make this function return the rotated text you can reuse it in these two cases (and more).

Getting the key

I suggest a function take the key and that will make sure that the input is valid and return it as its return value.

A function to rotate letters

I suggest making a function called rotated_letter which will rotate a letter and then return this rotated letter and we will use it in rotated_word. This will make the code more readable and reusable.

Key

I don't know why yoy don't accept 1 as a key. I am not an expert about cryptography, but I think it can be accepted. So I suggest you accept numbers between 1 and 25.

Decoding

You mentioned that your code codes a text or decodes it. It does the first part but doesn't do the second. I therefore suggest making a function to get the mode.

Calling other functions

I suggest making a function called main and using if __name__ == "__main__", as was suggested, to call it. In main we would call other functions and put all global variables.

Here is your code after refactoring:

def rotated_letter(letter, key):
    """Rotates a given letter by a given number.

    letter: str

    key: int

    Returns: str
    """
    if letter.isalpha():    
        num = ord(letter)

        if (num + key) > 122: 
            x = (num + key) - 122
            return chr(x + ord('a') - 1)
        else:
            return chr(num + key)

    else:
            return letter


def rotated_word(word, key):
    """Rotates a word by a given key.

    word: str

    key: int

    Returns: str
    """
    new_word = ""

    for letter in word:
        new_letter = rotated_letter(letter, key)
        new_word += new_letter

    return new_word


def get_key():
    """Asks for an integer between 1 and 25.

    Returns: int
    """
    while True:   
        try:
            key = int(input('Input the key you want to use from  1 to 25:\n'))

            if key in range(1, 26):
                return key

        except:
            print("\nPlease enter an integer.\n")


def get_mode():
    """Asks for a mode (coding or decoding).

    Returns: str
    """
    while True:
        mode = input('Enter C to code or D to decode:\n').upper()

        if mode in ('C', 'D'):
            return mode


def main():
    word =  input('Input the text you want to code:\n').lower()

    key = get_key()

    mode = get_mode()

    if mode == 'C':
        print(rotated_word(word, key))
    else:
        print(rotated_word(word, -key))


if __name__ == "__main__":
    main()

Notes

I depended on some points from ChatterOne's answer, and edited somethings according to it.

\$\endgroup\$
2
\$\begingroup\$

Do not try to reinvent the wheel because Python comes with a built-in rot13 algorithm to encode and decode. You can use it by relying on codecs module:

>>> import codecs
>>> codecs.encode('Hello World!', 'rot13')
'Uryyb Jbeyq!'
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I don't think it's bad to reinvent the wheel in order to practice. In Think Python the author would make a function that already exists and then would come and say that there is a built-in function that does this. I find reinventing the wheel very helpful, sometimes. \$\endgroup\$ Mar 4, 2017 at 18:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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