Basic shift cipher in Python

Clash 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)
python beginner python-3.x caesar-cipher
add a comment |Â
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)
python beginner python-3.x caesar-cipher
add a comment |Â
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)
python beginner python-3.x caesar-cipher
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)
python beginner python-3.x caesar-cipher
edited Jun 28 at 6:16
200_success
123k14143399
123k14143399
asked Jun 28 at 5:23
j-grimwood
334
334
add a comment |Â
add a comment |Â
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))
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
add a comment |Â
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.
add a comment |Â
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))
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
add a comment |Â
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))
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
add a comment |Â
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))
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))
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
add a comment |Â
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
add a comment |Â
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.
add a comment |Â
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.
add a comment |Â
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.
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.
answered Jun 28 at 7:32
Krzysztof
715
715
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
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
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password