# Simulated card trick with splitting and reordering

I need a bit of help with a practice Computer Science coursework. I have written a code for a magic trick (more information below) using Python 3.x, and for one particular section, I need to suggest potential improvements/enhancements. I know this sounds a tiny bit egotistical, but I can't seem to think of any :( .

The program is called ‘The Card Trick’. It simulates a magic trick that can be performed in real-life. The program should generate 21 random playing cards, with suits and values, and will split them into three groups of seven. The user will be asked to choose a card, and input which group the chosen card is contained within. The cards will then be put back into a larger list in a certain order, and dealt in the same way as before. The steps will be repeated twice, and then the program will ‘magically’ print the card that the user is thinking of.

Anyway, here is my code:

import random # Imports the random module

def get_cards():
'''
Funtion which randomly generates the cards and adds them to
deck if the card is not in the deck already.
Parameters: None
Returns: cards_dealt
'''
cards_dealt=[]
while len(cards_dealt) < 21: # Repeats until there are 21 cards in the deck
#Generates the card
card = random.choice(['A','2','3','4','5','6','7','8','9','10','J','Q','K'])+ random.choice([u'\u2665',u'\u2663',u'\u2666',u'\u2660'])
if card not in cards_dealt: # If card not already in the deck
cards_dealt.append(card) # Adds card to deck
else:
continue # continue generating cards
return cards_dealt

def get_piles(deck):
'''
Funtion which deals the cards in the way that a magician would.
It then adds the cards to three lists called P1, P2 and P3, depending
on their position in the deck.
Parameters: deck
Returns: P1, P2, P3
'''                                   # range() makes lists of:
P1 = [deck[i] for i in range(0,21,3)] # [0,3,6,9,12,15,18]
P2 = [deck[i] for i in range(1,21,3)] # [1,4,7,10,13,16,19]
P3 = [deck[i] for i in range(2,21,3)] # [2,5,8,11,15,17,20] which correspond to the positons of cards in the deck.
return P1, P2, P3

def get_newdeck(choice, P1, P2, P3):
'''
Function which reorders the deck in a way that the chosen list is
in the middle of the two other lists.
Parameters: choice, P1, P2, P3
Returns: the new, reordered deck
'''
deck = P1+P3+P2 # Orders the deck with Pile 3 in middle
if choice == 1: # if user's choice is 1
deck = P2+P1+P3 # Put pile 1 between other piles
elif choice == 2: # If user's choice is 2
deck = P1+P2+P3 # Put pile 2 between other piles
return deck

def PrintPiles(P1, P2, P3):
'''
Procedure which prints the lists(P1, P2, and P3) vertically
with headers to tell the user which piles they are.
Parameters: P1, P2, P3
Returns: None
'''
# Prints the piles vertically
print("Pile 1\tPile 2\tPile 3")
for i in range(7):
print(P1[i]+'\t'+P2[i]+'\t'+P3[i])

def Get_Choice():
'''
Funtion which gets the users input of which pile their chosen
cards is inside. It also tells the user if they entered an invalid
input (not 3, 2, or 1).
Parameters: None
Returns: choice
'''
choice = 0 # sets variable choice to 0
# while loops and try:except fix invalid inputs
while choice > 3 or choice < 1: # Choice not 1-3
while True: # Allows the user to keep entering inputs when input invalid
try:
choice = int(input("Which pile is your card in? (1-3)? > "))
break
except ValueError: # If input non-integer and returns exception
# ValueError
print("Must be an integer")
return choice

def main():
'''
The main body of the code, using previous procedures and functions
to make the code work as intended.
Parameters: None
Returns: None
'''
deck = get_cards() # generates deck (list) from get_cards function
print("Choose a card and remember it")
# Repeats 3 times
for x in range(3):
# 'deals' three piles and stores them in P1, P2 and P3
P1, P2, P3 = get_piles(deck)
# calls PrintPiles procedure
PrintPiles(P1, P2, P3)
# gets the user's choice between 1 and 3 depending on
# the position of their card
choice = Get_Choice()
print()
# reorders the deck according to the user's choice
deck = get_newdeck(choice, P1, P2, P3)
print()
# Prints the card in the middle of the deck
print('>>> Your card is {} <<<'.format(deck[10]))

main()


Any ideas? FYI no improvements regarding commenting please! Believe me - I tried incredibly hard to find anything, my best guess was to try to improve the speed, but it already runs pretty well. I suppose I could use some different data structures to make my code more concise.

EDIT - Thanks to you all. If I could accept all of the answers, I would. One small thing - I noticed that, when I made a separate python file with all improvements suggested, the file size went down from 4 KB to 3 KB! With this check mark, assume it is all of your answers I am accepting :P
In the following code, I used something from all of your answers as a small thank you. Please feel free to try the code yourself!
New Code:

import random

def get_deck():
'''
Funtion which randomly selects the cards from a full
deck of 52 cards previously generated.
Parameters: None
Returns: a list of 21 cards
'''
ranks = ['A','2','3','4','5','6','7','8','9','10','J','Q','K']
suits = [u'\u2665',u'\u2663',u'\u2666',u'\u2660']
full_deck = [r+s for s in suits for r in ranks] # Creates a full deck of 52 cards
return random.sample(full_deck, 21) # returns a random sample of 21 cards from the deck

def get_piles(deck):
'''
Funtion which deals the cards in the way that a magician would.
It then adds the cards to three lists called pile_1, pile_2 and pile_3, depending
on their position in the deck.
Parameters: deck
Returns: pile_1, pile_2, pile_3
'''
pile_1 = deck[0::3] # Used python's list slicing to split into piles
pile_2 = deck[1::3] # rather than the for loop used before
pile_3 = deck[2::3]
return pile_1, pile_2, pile_3

def get_newdeck(choice, pile_1, pile_2, pile_3):
'''
Function which reorders the deck in a way that the chosen list is
in the middle of the two other lists.
Parameters: choice, pile_1, pile_2, pile_3
Returns: the new, reordered deck
'''
deck = pile_1+pile_3+pile_2
if choice == 1:
deck = pile_2+pile_1+pile_3
elif choice == 2:
deck = pile_1+pile_2+pile_3
return deck

def print_piles(pile_1, pile_2, pile_3):
'''
Procedure which prints the lists(pile_1, pile_2, and pile_3) vertically
with headers to tell the user which piles they are.
Parameters: pile_1, pile_2, pile_3
Returns: None
'''
print("Pile 1\tPile 2\tPile 3")
for i in range(7):
print(pile_1[i]+'\t'+pile_2[i]+'\t'+pile_3[i])

def get_choice():
'''
Funtion which gets the user's input of which pile their chosen
cards is inside. It also tells the user if they entered an invalid
input (not 3, 2, or 1). It also allows the user to quit the program.
Parameters: None
Returns: choice
'''
while True:
try:
choice = int(input("Which pile is your card in? (1-3), press CTRL + C to quit? > "))
except ValueError:
print("Must be an integer")
except KeyboardInterrupt: # When user presses Ctrl + C
print("\nThanks... good bye!")
quit(0) # Terminates the program
else:
if 1 <= choice <= 3:
break
else:
print("Must be between 1 and 3")
return choice

def main():
'''
The main body of the code, using previous procedures and functions
to make the code work as intended.
Parameters: None
Returns: None
'''
deck = get_deck()
print("Choose a card and remember it")
for x in range(3):
pile_1, pile_2, pile_3 = get_piles(deck) # Changed function and variable names
print_piles(pile_1, pile_2, pile_3)
choice = get_choice()
print()
deck = get_newdeck(choice, pile_1, pile_2, pile_3)
print()
print('>>> Your card is {} <<<'.format(deck[10]))

if __name__ == '__main__':
main()

• As per the help center, you must be willing to accept criticism on any aspect of the code — including the commenting. (On the other hand, if your instructor forces you to write comments that you personally find disagreeable, please explain.) Nov 17, 2017 at 22:22
• Yes, that is fine. It is just that I have to write up the last part of the coursework in word form, as I have already finished the programming section. The part that I am doing is about improvements for the performance/aesthetics/readability of the code, not the comments Nov 17, 2017 at 23:02

explicit loops

python has list slicing and list comprehension. in get_piles(deck) you use comprehension but could use the more dense slicing

P1 = [deck[i] for i in range(0,21,3)] # [0,3,6,9,12,15,18]
P2 = [deck[i] for i in range(1,21,3)] # [1,4,7,10,13,16,19]
P3 = [deck[i] for i in range(2,21,3)] # [2,5,8,11,15,17,20] which correspond to the positons of cards in the deck.


rewrites to

P1 = deck[0::3]
P2 = deck[1::3]
P3 = deck[2::3]


also get_cards has an explicit loop which you can do much more pythonic (and much more readable)

def get_cards():
# ...
cards_dealt=[]
while len(cards_dealt) < 21: # Repeats until there are 21 cards in the deck
#Generates the card
card = random.choice(['A','2','3','4','5','6','7','8','9','10','J','Q','K'])+ random.choice([u'\u2665',u'\u2663',u'\u2666',u'\u2660'])
if card not in cards_dealt: # If card not already in the deck
cards_dealt.append(card) # Adds card to deck
else:
continue # continue generating cards
return cards_dealt


rewrites to

def get_deck():
ranks = ['A','2','3','4','5','6','7','8','9','10','J','Q','K']
suits = [u'\u2665',u'\u2663',u'\u2666',u'\u2660']
full_deck = [r+s for s in suits for r in ranks]
return random.sample(full_deck, 21)


naming conventions (and PEP 8)

you use different styles of naming (underscore, upper/lower/camel case)

def get_newdeck(choice, P1, P2, P3):
def PrintPiles(P1, P2, P3):
def Get_Choice():


this is hard to read. the first one complies with the PEP 8 convention, stick to that.

setting default values

in get_newdeck you set a default value which is then overwritten in 2 cases. this is a common and accepted pattern if the default value is a simple value (True, 0, ..) which is overwritten in a somewhat complicated decision tree. however in your case all three branches are of equal rank, so instead of

def get_newdeck(choice, P1, P2, P3):
# ...
deck = P1+P3+P2 # Orders the deck with Pile 3 in middle
if choice == 1: # if user's choice is 1
deck = P2+P1+P3 # Put pile 1 between other piles
elif choice == 2: # If user's choice is 2
deck = P1+P2+P3 # Put pile 2 between other piles
return deck


i would prefer

def get_newdeck(choice, P1, P2, P3):
# ...
if choice == 1: # if user's choice is 1
deck = P2+P1+P3 # Put pile 1 between other piles
elif choice == 2: # If user's choice is 2
deck = P1+P2+P3 # Put pile 2 between other piles
else:
deck = P1+P3+P2 # Orders the deck with Pile 3 in middle
return deck


but as always there is a more dense version (while some may prefer the previous as more readable)

def get_newdeck(choice, P1, P2, P3):
# ...
piles = [P1,P2,P3]*3)[choice+1:choice+4]
return [card for pile in piles for card in pile]


indices

in get_newdeck you use a parameter choice which is one-based. most programming language use zero-based indics (offset from the first element). while on the user interface it is natural to use [1,2,3] as choice you should immediately convert this to a zero-based internal representation. this is what other programmers expect (or even yourself in a year) when maintaining your code.

in get_newdeck you use a parameter choice which is one-based. this is not common and not mentioned in the docstring. so while there is a 6 line docstring one of the most important facts for correct usage is not mentioned. While your parameters and return values are listed by name there is no hint about the correct type nor the value range.

as this is a very lengthy list of more or less important issues i have to say this still is a very good start

• the code is structured(!) in testable(!) functions
• there are docstrings describing the intended functionality
• there is separation(!) between I/O (user interface) and core functionality

programming is about continous learning and getting better.

# Review

• Your def get_cards(): is a bit funky, do you want to have a full deck? Because at the moment you only have 21 cards in the deck.
• There is a shuffle() method in the random module that shuffles a list

Therefore I would rewrite your get_cards() as this:

def get_cards():
'''Creates a deck of cards and shuffle them'''
suits = [u'\u2665',u'\u2663',u'\u2666',u'\u2660']
cards = ['A','2','3','4','5','6','7','8','9','10','J','Q','K']
deck = ["{0}{1}".format(card, suit) for card in cards for suit in suits]
random.shuffle(deck)

# if you do really want to have only 21 cards
#You could return a sliced list like this deck[:21]
return deck


• functions and variable names should be snake_case see PEP8 for more styling in Pyhton.
• P1 should be renamed pile_1 for instance
• PrintPiles should be print_piles etc...

Your double while loop looks silly to me, I think it be more concise to use 1 while True: loop and use a if else structure within

def get_choice():
'''
Funtion which gets the users input of which pile their chosen
cards is inside. It also tells the user if they entered an invalid
input (not 3, 2, or 1).
Parameters: None
Returns: choice
'''
while True:
try:
choice = int(input("Which pile is your card in? (1-3)? > "))
if 1 <= choice <= 3:
break
else:
print("Must be between 1 and 3")
except ValueError:
print("Must be an integer")
return choice


• use a if __name__ == '__main__' body.

Let's take for instance this piece of code:

    if card not in cards_dealt: # If card not already in the deck
cards_dealt.append(card) # Adds card to deck
else:
continue # continue generating cards

• Here the comments are entirely redundant, and you should just remove them. Your code is easily understandable without them. It just ads noise to the code. This happens way too much now.
• Good on you for having some great '''docstrings''' which do a much better job of telling what the code does.
• OP didn't mention using Python 3.6, but f-strings are 3.6+ only. Maybe change that to str.format again? Nov 18, 2017 at 10:37
• @Coal_ Yeah I should. I guess f"{strings}" is becoming the standard way to write it for me ^^ Nov 18, 2017 at 10:42
• FYI i did want the 21 cards, as only 21 are needed for the trick, but thank you anyway. Just assume your answer is accepted as well Nov 18, 2017 at 15:45

Your functions are good examples of what a function should really be: sort, doing one thing only and well. This rule leads to clean code.

I wrote "and well" instead of "and well" because there have been already few pitfalls highlighted in the previous answer and here are fewer more:

• In get_cards(), it is unnecessary to write:

else:
continue # continue generating cards


because moving to the next iteration is done already automatically given your coding context.

• @Ludisposed is right about the while statements within get_choice() function. However, I would like to go further because when I compare your try to else statement, I find the later one is lighter. This attitude is in opposite to the the common good practice where the else block helps you minimize the amount of code in the try block and improves readability.

• Let us stay within the get_choice() function: It is also advised to follow this schema if/except/else instead of the one you coded (if/else/except).

So given the last 2 observations above, I would re-write your function this way:

def get_choice():
while True:
try:
choice = int(input("Which pile is your card in? (1-3)? > "))
except ValueError:
print("Must be an integer")
else:
if 1 <= choice <= 3:
break
else:
print("Must be between 1 and 3")
return choice


Now, what if the user aims to exit your application in the middle of get_choice() execution? The Ctrl + C natural reflex spits out an exception instead of a smooth Good bye!. Let us deal with it:

def get_choice():
while True:
try:
choice = int(input("Which pile is your card in? (1-3)? > "))
except ValueError:
print("Must be an integer")
except KeyboardInterrupt:
print("\nThanks... good bye!")
quit(0)
else:
if 1 <= choice <= 3:
break
else:
print("Must be between 1 and 3")
return choice

• You followed the snake_case naming convention for your functions, except for 2 of them: Get_Choice() and PrintPiles(); the later one is a typical way for naming Python classes.