Simulated card trick with splitting and reordering

Question

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()

Answer 1

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.

---

comments and docstrings

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.

Answer 2

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.

---

You don't want to hear this but let's talk about your comments

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.

Answer 3

In addition to the existing answers, here is my short observation:

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.
• Last point: your code is fairly readable, IMHO you could remove all the comments.