Basic shift cipher in Python

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
6
down vote

favorite












I am a bit new to Python. This is my first code review, and I am excited to get a critique on how to make this (very simple) shift cipher more Pythonic, cleaner, and more organized.



alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j','k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

def shift(offset):

message = input("Input Message You Would Like Encrypted:n")
new_message = ''

for letter in message:

letter = letter.lower() #doesn't handle upper-case yet

if letter.isalpha():
shift_pos = alphabet.index(letter) + offset
new_pos = alphabet[shift_pos]
new_message += new_pos

#these will not be shifted

elif ' ' or '/t' or '/n' in letter:
new_message += letter

elif letter.isnumeric():
new_message += letter

else:
print("An error took place in recording the message. Check input.n")

print(new_message)


shift(-1)






share|improve this question



























    up vote
    6
    down vote

    favorite












    I am a bit new to Python. This is my first code review, and I am excited to get a critique on how to make this (very simple) shift cipher more Pythonic, cleaner, and more organized.



    alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j','k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

    def shift(offset):

    message = input("Input Message You Would Like Encrypted:n")
    new_message = ''

    for letter in message:

    letter = letter.lower() #doesn't handle upper-case yet

    if letter.isalpha():
    shift_pos = alphabet.index(letter) + offset
    new_pos = alphabet[shift_pos]
    new_message += new_pos

    #these will not be shifted

    elif ' ' or '/t' or '/n' in letter:
    new_message += letter

    elif letter.isnumeric():
    new_message += letter

    else:
    print("An error took place in recording the message. Check input.n")

    print(new_message)


    shift(-1)






    share|improve this question























      up vote
      6
      down vote

      favorite









      up vote
      6
      down vote

      favorite











      I am a bit new to Python. This is my first code review, and I am excited to get a critique on how to make this (very simple) shift cipher more Pythonic, cleaner, and more organized.



      alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j','k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

      def shift(offset):

      message = input("Input Message You Would Like Encrypted:n")
      new_message = ''

      for letter in message:

      letter = letter.lower() #doesn't handle upper-case yet

      if letter.isalpha():
      shift_pos = alphabet.index(letter) + offset
      new_pos = alphabet[shift_pos]
      new_message += new_pos

      #these will not be shifted

      elif ' ' or '/t' or '/n' in letter:
      new_message += letter

      elif letter.isnumeric():
      new_message += letter

      else:
      print("An error took place in recording the message. Check input.n")

      print(new_message)


      shift(-1)






      share|improve this question













      I am a bit new to Python. This is my first code review, and I am excited to get a critique on how to make this (very simple) shift cipher more Pythonic, cleaner, and more organized.



      alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j','k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

      def shift(offset):

      message = input("Input Message You Would Like Encrypted:n")
      new_message = ''

      for letter in message:

      letter = letter.lower() #doesn't handle upper-case yet

      if letter.isalpha():
      shift_pos = alphabet.index(letter) + offset
      new_pos = alphabet[shift_pos]
      new_message += new_pos

      #these will not be shifted

      elif ' ' or '/t' or '/n' in letter:
      new_message += letter

      elif letter.isnumeric():
      new_message += letter

      else:
      print("An error took place in recording the message. Check input.n")

      print(new_message)


      shift(-1)








      share|improve this question












      share|improve this question




      share|improve this question








      edited Jun 28 at 6:16









      200_success

      123k14143399




      123k14143399









      asked Jun 28 at 5:23









      j-grimwood

      334




      334




















          2 Answers
          2






          active

          oldest

          votes

















          up vote
          13
          down vote



          accepted










          Bug




          elif ' ' or '/t' or '/n' in letter: 
          new_message += letter



          Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string. The rest of the expression doesn't matter due to short-circuit evaluation of or. For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.



          Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.



          Did you mean to write this instead?



          elif letter in ' tn':
          new_message += letter


          Naming



          letter might not be a letter of the alphabet. A better name would be character, char, or just c.



          Design



          shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it). Then, you would call



          print(shift(input("Input message you would like encrypted:n"), -1))


          Why do you want to disallow certain characters? If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?



          Efficiency



          Strings in Python are immutable. Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending. Therefore, += is a relatively expensive operation and should be avoided.



          A better alternative would be to build a list (by appending or using a list comprehension). Then, you construct the result by calling ''.join(…).



          Pythonic cleverness



          Your alphabet could just be string.ascii_lowercase instead of that list.



          The classic way to implement a Caesar Cipher in Python is using str.translate().



          from string import ascii_lowercase as ALPHABET

          def shift(message, offset):
          trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
          return message.lower().translate(trans)

          print(shift(input("Input message you would like encrypted:n"), -1))





          share|improve this answer





















          • I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
            – j-grimwood
            Jun 28 at 19:42

















          up vote
          3
          down vote













          Non ascii characters



          You should check out what will happen if your string to cypher would include non ascii character.
          For example polish letter 'ą'. It is alpha character and it will go into check of index, but as this value is not included in the list of characters trying to get it's index will result with Value Error.



          Tests



          Code is easier to understand and be checked if there are tests included.
          Write up some tests to check if the algorithm actually works.






          share|improve this answer





















            Your Answer




            StackExchange.ifUsing("editor", function ()
            return StackExchange.using("mathjaxEditing", function ()
            StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
            StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
            );
            );
            , "mathjax-editing");

            StackExchange.ifUsing("editor", function ()
            StackExchange.using("externalEditor", function ()
            StackExchange.using("snippets", function ()
            StackExchange.snippets.init();
            );
            );
            , "code-snippets");

            StackExchange.ready(function()
            var channelOptions =
            tags: "".split(" "),
            id: "196"
            ;
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function()
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled)
            StackExchange.using("snippets", function()
            createEditor();
            );

            else
            createEditor();

            );

            function createEditor()
            StackExchange.prepareEditor(
            heartbeatType: 'answer',
            convertImagesToLinks: false,
            noModals: false,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            );



            );








             

            draft saved


            draft discarded


















            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f197397%2fbasic-shift-cipher-in-python%23new-answer', 'question_page');

            );

            Post as a guest






























            2 Answers
            2






            active

            oldest

            votes








            2 Answers
            2






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes








            up vote
            13
            down vote



            accepted










            Bug




            elif ' ' or '/t' or '/n' in letter: 
            new_message += letter



            Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string. The rest of the expression doesn't matter due to short-circuit evaluation of or. For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.



            Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.



            Did you mean to write this instead?



            elif letter in ' tn':
            new_message += letter


            Naming



            letter might not be a letter of the alphabet. A better name would be character, char, or just c.



            Design



            shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it). Then, you would call



            print(shift(input("Input message you would like encrypted:n"), -1))


            Why do you want to disallow certain characters? If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?



            Efficiency



            Strings in Python are immutable. Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending. Therefore, += is a relatively expensive operation and should be avoided.



            A better alternative would be to build a list (by appending or using a list comprehension). Then, you construct the result by calling ''.join(…).



            Pythonic cleverness



            Your alphabet could just be string.ascii_lowercase instead of that list.



            The classic way to implement a Caesar Cipher in Python is using str.translate().



            from string import ascii_lowercase as ALPHABET

            def shift(message, offset):
            trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
            return message.lower().translate(trans)

            print(shift(input("Input message you would like encrypted:n"), -1))





            share|improve this answer





















            • I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
              – j-grimwood
              Jun 28 at 19:42














            up vote
            13
            down vote



            accepted










            Bug




            elif ' ' or '/t' or '/n' in letter: 
            new_message += letter



            Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string. The rest of the expression doesn't matter due to short-circuit evaluation of or. For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.



            Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.



            Did you mean to write this instead?



            elif letter in ' tn':
            new_message += letter


            Naming



            letter might not be a letter of the alphabet. A better name would be character, char, or just c.



            Design



            shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it). Then, you would call



            print(shift(input("Input message you would like encrypted:n"), -1))


            Why do you want to disallow certain characters? If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?



            Efficiency



            Strings in Python are immutable. Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending. Therefore, += is a relatively expensive operation and should be avoided.



            A better alternative would be to build a list (by appending or using a list comprehension). Then, you construct the result by calling ''.join(…).



            Pythonic cleverness



            Your alphabet could just be string.ascii_lowercase instead of that list.



            The classic way to implement a Caesar Cipher in Python is using str.translate().



            from string import ascii_lowercase as ALPHABET

            def shift(message, offset):
            trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
            return message.lower().translate(trans)

            print(shift(input("Input message you would like encrypted:n"), -1))





            share|improve this answer





















            • I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
              – j-grimwood
              Jun 28 at 19:42












            up vote
            13
            down vote



            accepted







            up vote
            13
            down vote



            accepted






            Bug




            elif ' ' or '/t' or '/n' in letter: 
            new_message += letter



            Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string. The rest of the expression doesn't matter due to short-circuit evaluation of or. For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.



            Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.



            Did you mean to write this instead?



            elif letter in ' tn':
            new_message += letter


            Naming



            letter might not be a letter of the alphabet. A better name would be character, char, or just c.



            Design



            shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it). Then, you would call



            print(shift(input("Input message you would like encrypted:n"), -1))


            Why do you want to disallow certain characters? If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?



            Efficiency



            Strings in Python are immutable. Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending. Therefore, += is a relatively expensive operation and should be avoided.



            A better alternative would be to build a list (by appending or using a list comprehension). Then, you construct the result by calling ''.join(…).



            Pythonic cleverness



            Your alphabet could just be string.ascii_lowercase instead of that list.



            The classic way to implement a Caesar Cipher in Python is using str.translate().



            from string import ascii_lowercase as ALPHABET

            def shift(message, offset):
            trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
            return message.lower().translate(trans)

            print(shift(input("Input message you would like encrypted:n"), -1))





            share|improve this answer













            Bug




            elif ' ' or '/t' or '/n' in letter: 
            new_message += letter



            Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string. The rest of the expression doesn't matter due to short-circuit evaluation of or. For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.



            Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.



            Did you mean to write this instead?



            elif letter in ' tn':
            new_message += letter


            Naming



            letter might not be a letter of the alphabet. A better name would be character, char, or just c.



            Design



            shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it). Then, you would call



            print(shift(input("Input message you would like encrypted:n"), -1))


            Why do you want to disallow certain characters? If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?



            Efficiency



            Strings in Python are immutable. Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending. Therefore, += is a relatively expensive operation and should be avoided.



            A better alternative would be to build a list (by appending or using a list comprehension). Then, you construct the result by calling ''.join(…).



            Pythonic cleverness



            Your alphabet could just be string.ascii_lowercase instead of that list.



            The classic way to implement a Caesar Cipher in Python is using str.translate().



            from string import ascii_lowercase as ALPHABET

            def shift(message, offset):
            trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
            return message.lower().translate(trans)

            print(shift(input("Input message you would like encrypted:n"), -1))






            share|improve this answer













            share|improve this answer



            share|improve this answer











            answered Jun 28 at 6:54









            200_success

            123k14143399




            123k14143399











            • I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
              – j-grimwood
              Jun 28 at 19:42
















            • I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
              – j-grimwood
              Jun 28 at 19:42















            I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
            – j-grimwood
            Jun 28 at 19:42




            I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again.
            – j-grimwood
            Jun 28 at 19:42












            up vote
            3
            down vote













            Non ascii characters



            You should check out what will happen if your string to cypher would include non ascii character.
            For example polish letter 'ą'. It is alpha character and it will go into check of index, but as this value is not included in the list of characters trying to get it's index will result with Value Error.



            Tests



            Code is easier to understand and be checked if there are tests included.
            Write up some tests to check if the algorithm actually works.






            share|improve this answer

























              up vote
              3
              down vote













              Non ascii characters



              You should check out what will happen if your string to cypher would include non ascii character.
              For example polish letter 'ą'. It is alpha character and it will go into check of index, but as this value is not included in the list of characters trying to get it's index will result with Value Error.



              Tests



              Code is easier to understand and be checked if there are tests included.
              Write up some tests to check if the algorithm actually works.






              share|improve this answer























                up vote
                3
                down vote










                up vote
                3
                down vote









                Non ascii characters



                You should check out what will happen if your string to cypher would include non ascii character.
                For example polish letter 'ą'. It is alpha character and it will go into check of index, but as this value is not included in the list of characters trying to get it's index will result with Value Error.



                Tests



                Code is easier to understand and be checked if there are tests included.
                Write up some tests to check if the algorithm actually works.






                share|improve this answer













                Non ascii characters



                You should check out what will happen if your string to cypher would include non ascii character.
                For example polish letter 'ą'. It is alpha character and it will go into check of index, but as this value is not included in the list of characters trying to get it's index will result with Value Error.



                Tests



                Code is easier to understand and be checked if there are tests included.
                Write up some tests to check if the algorithm actually works.







                share|improve this answer













                share|improve this answer



                share|improve this answer











                answered Jun 28 at 7:32









                Krzysztof

                715




                715






















                     

                    draft saved


                    draft discarded


























                     


                    draft saved


                    draft discarded














                    StackExchange.ready(
                    function ()
                    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f197397%2fbasic-shift-cipher-in-python%23new-answer', 'question_page');

                    );

                    Post as a guest













































































                    Popular posts from this blog

                    Python Lists

                    Aion

                    JavaScript Array Iteration Methods