2
\$\begingroup\$

My teacher wants us to create a program that runs n games of war and calculates the average number of battles, wars and double wars that occurred. He has very specific rules (not necessarily the actual rules of war). I know there's some redundant code in the deck building and splitting for each game, but that's how he wants it.

His rules are as follows:

  • The start of a list should be considered the top of a deck.
  • Whenever cards are 'won', they should be shuffled together and placed on the bottom of the winner's deck.
  • Wars occur when the initial cards are identical. Each player deals three cards face down and picks one of those three at random to decide the war. If it's another tie, the process is repeated. If a player runs out of cards while putting three down, they use the last card they drew.
  • Lastly, each battle OR war counts as a battle, each war is counted separately, and the 'double war' stat is only incremented when a war lasted exactly two rounds.

While it's not perfect or efficient, I'm fairly certain my code works as per the specifications, but his sample results differ from mine. His battles, wars, and double wars are ~365, ~23, and ~1.3 respectively (over 1000 games). Mine are ~265, ~20, ~2.6.

import random

def buildDeck():
    '''Builds and returns a standard, shuffled deck of 52 playing cards.'''
    ranks = [str(x) for x in range(2, 11)] + ['J', 'Q', 'K', 'A']
    suits = ['D', 'H', 'S', 'C']
    deck = [rank + ' of ' + suit for rank in ranks for suit in suits]
    random.shuffle(deck)

    return deck

def cardValue(card):
    '''Receives a single card and returns the integer value of the card (2-14)'''
    faceCards = ['J', 'Q', 'K', 'A']

    if card[0] in faceCards:
        return faceCards.index(card[0]) + 11

    else:
        return int(card[0])



def cleanUp(pileOne, pileTwo):
    '''Receives both players piles and checks for and removes a placeholder in the last list index (empty item).'''
    if not pileOne[-1]:
        pileOne.pop()
    if not pileTwo[-1]:
        pileTwo.pop()



def cardChoice(pile):
    '''During a war, checks if placeholder is present in last index (meaning the player ran out of cards before putting three down). If present, return last actual card added. If not present, pick from the last three at random.'''
    if pile[-1]:
        return random.choice(pile[-3:])
    else:
        return pile[-2]



def replaceCards(deck, pileOne, pileTwo):
    '''Shuffles together both players piles and appends them to the bottom of the winners deck.'''
    pileOne.extend(pileTwo)
    random.shuffle(pileOne)
    deck.extend(pileOne)

def playWar():

    deck = buildDeck()
    battleStat, warStat, doubleWarStat = 0, 0, 0
    pileOne, pileTwo, deckOne, deckTwo = [], [], [], []

    for i in xrange(26):
        deckOne.append(deck.pop())
        deckTwo.append(deck.pop())

    while deckOne and deckTwo:

        del pileOne[:], pileTwo[:]
        pileOne.append(deckOne.pop(0))
        pileTwo.append(deckTwo.pop(0))

        if cardValue(pileOne[0]) > cardValue(pileTwo[0]):
            battleStat += 1
            replaceCards(deckOne, pileOne, pileTwo)

        elif cardValue(pileTwo[0]) > cardValue(pileOne[0]):
            battleStat += 1
            replaceCards(deckTwo, pileOne, pileTwo)

        else:

            warCount = 1

            while True:
                battleStat += 1
                warStat += 1

                for i in xrange(3):
                    if deckOne:
                        pileOne.append(deckOne.pop(0))
                    elif pileOne[-1]:
                        pileOne.append('')

                    if deckTwo:
                        pileTwo.append(deckTwo.pop(0))
                    elif pileTwo[-1]:
                        pileTwo.append('')

                if cardValue(cardChoice(pileOne)) > cardValue(cardChoice(pileTwo)):
                    cleanUp(pileOne, pileTwo)
                    replaceCards(deckOne, pileOne, pileTwo)

                    if warCount == 2:
                        doubleWarStat += 1
                    break

                elif cardValue(cardChoice(pileTwo)) > cardValue(cardChoice(pileOne)):
                    cleanUp(pileOne, pileTwo)
                    replaceCards(deckTwo, pileOne, pileTwo)

                    if warCount == 2:
                        doubleWarStat += 1
                    break

                warCount += 1

    return battleStat, warStat, doubleWarStat

def warStats(n):
    '''Records statistics from n number of War games.
       Prints out calculated averages for battles, wars, and double wars.'''

    battleStat, warStat, doubleWarStat = 0.0, 0.0, 0.0

    for i in range(n):
        result = playWar()
        battleStat += result[0]
        warStat += result[1]
        doubleWarStat += result[2]

    print '\nAvg # of battles: ' + str(battleStat/n)
    print 'Avg # of wars: ' + str(warStat/n)
    print 'Avg # of double wars: ' + str(doubleWarStat/n)



while True:
    selection = raw_input('\nHow many games of war would you like to play? (\'q\' to exit): ')

    try:
        if selection.lower()[0] == 'q':
            break

        elif 0 < int(selection):
            warStats(int(selection))

        else:
            print '\nPlease enter an integer between 1 and 1000'

    except:
        print '\nPlease enter an integer between 1 and 1000'
\$\endgroup\$
  • \$\begingroup\$ Apologies, I forgot to change out the Unicode characters and add comments. Cut out superfluous code to make it more readable initially and had forgotten some things. \$\endgroup\$ – Alex B Apr 14 '15 at 4:57
  • \$\begingroup\$ Requesting for help fixing broken code is off-topic; I've removed part of your question. \$\endgroup\$ – 200_success Apr 14 '15 at 5:50
  • \$\begingroup\$ My code is not broken, I was simply asking if there was an issue with my implementation as per the guidelines of the assignment that may explain the discrepancies. \$\endgroup\$ – Alex B Apr 14 '15 at 6:10
  • \$\begingroup\$ By Code Review standards, "wrong answer" = "broken code" = off-topic. If you want the question to be on-topic, then you can't ask for help changing the answer (though someone might be kind enough to spot a mistake anyway). \$\endgroup\$ – 200_success Apr 14 '15 at 6:16
2
\$\begingroup\$

Here are two things I spotted:

  • As you’ve written it, cardValue() read the first character of a string to determine the card’s value. But what if you get the card 10 of H? Apparently that has value 1, which is incorrect.

  • If I modify cardChoice() to swap the order (if a placeholder is present, return a choice of last three cards, otherwise return the last card put down), then I get the same statistics as your teacher. To be explicit:

    if pile[-1] == "":
        return random.choice(pile[-4:-1])
    else:
        return pile[-2]
    

    Note that I’ve had to adjust the range on the call to choice().

    It’s not totally clear to me why you should do this, but it does seem to match the teacher’s answer. ¯\_(ツ)_/¯

A few comments on general style:

  • Python function and variable names are lowercase_with_underscores. (See PEP 8, the Python style guide.)
  • It’s better to wrap your mainline function in a main() function, then have this block at the end of your script:

    if __name__ == '__main__':
        main()
    

    It allows you to import these functions in another script later: the main() function is only called if you run the script directly.

  • You should write more comments. I shouldn’t have to read the game design, and then try to apply that in the code. Explain how your code relates to the game: for example, “if the two cards are the same, then war occurs” followed by the code which I know now plays out war. Your comments should explain why you wrote a block of code.

    This will make it easier for other people to read your scripts, and for you to debug it later.

And now some comments on the meat of the script:

  • Your buildDeck() function constructs a list of strings, including the suit, and you then have to make lots of calls to cardValue() to turn the cards back into integers.

    Strings are easy for humans to read, but they don’t make a great way to store data. Store the int value directly. That also bypasses messy handling of values with picture cards.

    I see two possibilities:

    • You don’t use the suit data, so I think you can get rid of it. Just have your deck be a list of ints.
    • If you really need the suit data, store each card as a tuple (value, suit). You can then get the integer value of the card as card[0] rather than doing string parsing.

    I’d lean towards the former, and this is how I’d write it:

    def build_deck():
        """Builds and returns a standard, shuffled deck of 52 playing cards.
    
        Each card is represented as an integer from 1 to 13 (A=1). Cards are not
        distinguished by suit.
        """
        deck = range(1, 14) * 4
        random.shuffle(deck)
        return deck
    

    Note that this approach also saves you making any calls to cardValue().

  • In cardChoice(), you explain that you're using an empty string as a placeholder. I think it would be better to use Python's built-in None type as a placeholder nil value.

    Using None means you have to do explicit checks for a placeholder, rather than relying on Boolean coercion (which can be a source of subtle bugs).

  • In playWar(), there are a few places where I’d tidy things up.

    When you’re constructing deckOne and deckTwo, you can see that deckOne is going to be all the even-indexed cards, and deckTwo the odd-indexed cards. Rather than using a for loop, I think it looks better to use a list comprehension:

    deckOne = [deck[2 * i + 1] for i in range(26)]
    deckTwo = [deck[2 * i]     for i in range(26)]
    

    That may just be an aesthetic choice.

    Within the while loop, when you set up pileOne and pileTwo, I think it’s a bit cleaner to set them up like this:

    pileOne = [deckOne.pop(0)]
    pileTwo = [deckTwo.pop(0)]
    

    and drop the del pileOne[:], pileTwo[:] line.

    Combined with the other change, and it also means you can drop the initialisation line with [], [], [], [].

  • In warStats() and playWar(), you have loops with index variable i, where the value of the variable is unused within the loop; you just want to repeat an action a fixed number of times.

    In such a case, it’s common to use _ for the index variable; for example:

    for _ in xrange(n):
        result = playWar()
        battleStat += result[0]
        warStat += result[1]
        doubleWarStat += result[2]
    

    This makes it clear to the reader that the value of the index variable is unimportant.

\$\endgroup\$
  • \$\begingroup\$ Excellent response! To touch on the deck building and splitting methods, my teacher specified that I should write it as though someone might want to see the cards/suits, hence the additional steps. As for the formatting and structure, I heeded your advice and it looks much better now. Also, thanks for catching the 10 card value error. I initially had it as a split index rather than just an index but must have changed it at some point. Finally, I applaud you for finding an (albeit incorrect) way to get my teacher's result. I'll be sure to raise that point with him today. Thanks again! \$\endgroup\$ – Alex B Apr 14 '15 at 18:36
  • \$\begingroup\$ Update: My instructor found the error in my code. During wars, I was picking a new card to decide the winner each time, rather than setting the chosen card beforehand and then comparing. The change alexwlchan made to reproduce the results was effectively doing the same thing (albeit in the wrong way). \$\endgroup\$ – Alex B Apr 18 '15 at 14:00
1
\$\begingroup\$

Hint: What is cardValue('10 of C')?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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