10
\$\begingroup\$

I've written some Python code to generate a random hexadecimal string using 31 playing cards drawn without replacement (so no card appears more than once). This is sufficient to provide more than 160 random bits, which is considered sufficient for a bitcoin private key.

I'm interested to know whether anything in the code or the way it is presented would make life difficult for a programmer using it elsewhere. I would like to hear ways that it can be improved in this specific respect - ease of re-use. Any other types of criticism are also welcome.

The full code is here and I include the main functional parts below (I've omitted the long doc string and exception classes).

What would make this more useful for other programmers?

from math import factorial

factorial52 = factorial(52)
upperLimit = factorial52//factorial(52-31) - 1
cardRanks = "A23456789TJQK"
cardSuits = "SHDC"
cardCharacters = cardRanks + cardSuits
hexCharacters = "0123456789ABCDEF"
recognisedCharacters = set(i for i in (cardCharacters + hexCharacters))
allCards = []
for s in cardSuits:
    for t in cardRanks:
        allCards.append(t+s)

def request_input():
    print(
        "\n"
        "Enter a list of 31 playing cards in the format\n"
        "AS 2H 3D 4C 5S 6H 7D 8C 9S TH JD QC KS ...\n"
        "\n"
        "or a hexadecimal number in the range\n"
        "0 to 114882682E46B11EADE9F57C1E3E0BBD47FFFFFFF (52! / (52-31)! - 1)\n"
        "\n"
        "In either case you may include spaces or not as you wish.\n"
        "Use T rather than 10.  For example TH for ten of hearts.\n"
        "Upper and lower case letters are equivalent.\n"
        "\n"
        )
    return input()

def process(argument):
    """Convert the argument from cards to hex or hex to cards.

    Decide whether the argument is hexadecimal or a list of cards.
    Ambiguity is possible, as some of the characters used to 
    represent cards are valid hexadecimal characters.  However, a 
    valid list of 31 cards will contain no duplicate cards, and will 
    therefore contain some hearts or spades, represented by H or S.  
    A valid list of 31 cards will therefore never be a valid 
    hexadecimal string.
    """
    cleanArgument = nonwhitespace(argument).upper()
    check_for_unrecognised_characters(cleanArgument)
    try:
        value = int(cleanArgument, 16)   # Gives error if not hex.
    except ValueError:
        print(convert_to_hex(cleanArgument))
    else:
        print(convert_to_cards(value))

def nonwhitespace(argument):
    """Return argument with all whitespace removed.

    This includes removing any single spaces within the string.
    """
    return "".join(argument.split())

def check_for_unrecognised_characters(argument):
    """Raise an exception if a character is unrecognised.

    The only recognised characters in this context are hexadecimal 
    characters and card ranks and suits.
    """
    for i in argument:
        if i not in recognisedCharacters:
            message = ("Character '" + i + 
                "' not recognised as part of card or hexadecimal.")
            raise UnrecognisedCharacterError(message)

def convert_to_hex(argument):
    check_number_of_characters(argument)
    listOfCards = [argument[i:i+2] for i in range(0, 62, 2)]
    check_if_cards(listOfCards)
    check_for_card_repetition(listOfCards)
    return hex_representation(listOfCards)

def convert_to_cards(value):
    check_hex_is_in_range(value)
    return corresponding_cards(value)

def check_number_of_characters(argument):
    """Raise an exception if not exactly 31 cards."""
    length = len(argument)
    if length < 62:
        message = (
            "31 cards required, each 2 characters.\n"
            "62 characters required in total.\n"
            "Only " + str(length) + " nonwhitespace characters provided."
            )
        raise TooFewCardsError(message)
    if length > 62:
        message = (
            "31 cards required, each 2 characters.\n"
            "62 characters required in total.\n"
            "" + str(length) + " nonwhitespace characters provided."
            )
        raise TooManyCardsError(message)

def check_if_cards(listOfCards):
    """Raise an exception if not valid cards.

    Every card should be a rank character followed by a suit character.
    """
    for i in listOfCards:
        if i[0] not in cardRanks:
            message = (
                "'" + str(i) + "' is not a recognised card rank.\n"
                "A valid rank is a single character as follows:\n"
                "'A' (ace)\n"
                "'2' (two)\n"
                "'3' (three)\n"
                "'4' (four)\n"
                "'5' (five)\n"
                "'6' (six)\n"
                "'7' (seven)\n"
                "'8' (eight)\n"
                "'9' (nine)\n"
                "'T' (ten)\n"
                "'J' (jack)\n"
                "'Q' (queen)\n"
                "'K' (king)"
                )
            raise UnrecognisedCardRankError(message)
        if i[1] not in cardSuits:
            message = (
                "'" + str(i) + "' is not a recognised card suit.\n"
                "A valid suit is a single character as follows:\n"
                "'S' (spades)\n"
                "'H' (hearts)\n"
                "'D' (diamonds)\n"
                "'C' (clubs)"
                )
            raise UnrecognisedCardSuitError(message)

def check_for_card_repetition(listOfCards):
    """Check that there are 31 unique pairs of characters.

    The list is already known to contain exactly 31 pairs.  Just check 
    that each is unique.
    """
    uniqueCards = set(listOfCards)
    if not len(uniqueCards) == 31:
        message = (
            "No two cards should be the same.\n"
            "Cards should be drawn from a single deck of 52 cards.\n"
            "Cards should be drawn without replacement."
            )
        raise DuplicatedCardsError(message)

def check_hex_is_in_range(value):
    """Check 0 <= value <= 52!/(52-31)! - 1

    As Python's arbitrary precision integers cannot be represented
    in hexadecimal as negative without using a minus sign, which has
    already been precluded, check only the upper limit.
    """
    if value > upperLimit:
        message = (
            "The hexadecimal value is too large to be represented by 31 cards.\n"
            "The maximum valid value is 52!/(52-31)! - 1\n"
            "In hexadecimal this maximum is\n"
            "114882682E46B11EADE9F57C1E3E0BBD47FFFFFFF"
            )
        raise HexValueTooLargeError(message)

def hex_representation(listOfCards):
    """Return a hexadecimal string defined by the 31 cards.

    The 52 cards in the full deck are numbered from 0 to 51.
    The order used here is defined by allCards.
    The 1st card in listOfCards therefore gives a number from 0 to 51.
    Remove this card from the deck so the deck is now numbered from 
    0 to 50.
    The 2nd card in listOfCards now gives a number from 0 to 50.
    Continue in the same way, to convert the 3rd card to a number 
    from 0 to 49, and so on.
    The final (31st) card will be converted to a number from 0 to 21.
    There is now a list of 31 numbers, each in a smaller range than 
    the last.
    Keep the 1st number as it is (multiply by 1).
    Multiply the 2nd number by 52.
    Multiply the 3rd number by 52 * 51.
    Multiply the 4th number by 52 * 51 * 50.
    Continue until all 31 numbers have been updated in this way.
    The required result is the sum of this list of 31 numbers.
    """
    listOfNumbers = []
    deck = allCards
    for card in listOfCards:
        number = deck.index(card)
        listOfNumbers.append(number)
        deck.remove(card)
    for n in range(31):
        listOfNumbers[n] *= factorial52 // factorial(52-n)
    result = sum(listOfNumbers)
    return hex(result)[2:]

def corresponding_cards(value):
    """Return a string of 31 cards representing the value.

    Divide the value by 52 making a note of quotient and remainder.
    The remainder will be a number from 0 to 51.
    Start a list of numbers with this remainder.
    Use the quotient to continue the process.
    Divide the quotient by 51 making a note of quotient and remainder.
    The remainder will be a number from 0 to 50.
    Append this remainder to the list of numbers.
    Use the new quotient to continue the process.
    Continue until the list contains 31 numbers.
    The 1st number will be from 0 to 51, and defines a card from the 
    full deck of 52 cards (in the order defined by allCards).
    The 2nd number will be from 0 to 50, and defines a card from the 
    remaining 51 cards.
    Continue in the same way to convert the remaining numbers to cards.
    """
    deck = allCards
    listOfNumbers = []
    listOfCards = []
    for i in range(31):
        divisor = 52 - i
        quotient = value // divisor
        remainder = value % divisor
        listOfNumbers.append(remainder)
        value = quotient
    for cardNumber in listOfNumbers:
        card = deck.pop(cardNumber)
        listOfCards.append(card)
    return " ".join(listOfCards)

# Handle the case where this program is called from the command line.
if __name__ == "__main__":
    import sys
    arguments = sys.argv
    if len(arguments) < 2:
        argument = request_input()
    else:
        argument = "".join(arguments[1:])
    process(argument)

I'd also be interested in any feedback on the way I've phrased the question and the amount of code I've left out and left in.

\$\endgroup\$
6
\$\begingroup\$
from math import factorial

factorial52 = factorial(52)

Why do this? Its probably better to just call factorial(52) in the next line

upperLimit = factorial52//factorial(52-31) - 1

The python convention is make constants in ALL_CAPS

cardRanks = "A23456789TJQK"
cardSuits = "SHDC"
cardCharacters = cardRanks + cardSuits
hexCharacters = "0123456789ABCDEF"
recognisedCharacters = set(i for i in (cardCharacters + hexCharacters))

This is the same as set(cardCharacters + hexCharacters) except slower.

allCards = []
for s in cardSuits:
    for t in cardRanks:
        allCards.append(t+s)

You can use allCards = list(itertools.product(cardSuits, cardRanks)).

def request_input():
    print(
        "\n"
        "Enter a list of 31 playing cards in the format\n"
        "AS 2H 3D 4C 5S 6H 7D 8C 9S TH JD QC KS ...\n"
        "\n"
        "or a hexadecimal number in the range\n"
        "0 to 114882682E46B11EADE9F57C1E3E0BBD47FFFFFFF (52! / (52-31)! - 1)\n"
        "\n"
        "In either case you may include spaces or not as you wish.\n"
        "Use T rather than 10.  For example TH for ten of hearts.\n"
        "Upper and lower case letters are equivalent.\n"
        "\n"
        )
    return input()

def process(argument):

process and argument are both very generic names. Its better to have a more informative name.

    """Convert the argument from cards to hex or hex to cards.

    Decide whether the argument is hexadecimal or a list of cards.
    Ambiguity is possible, as some of the characters used to 
    represent cards are valid hexadecimal characters.  However, a 
    valid list of 31 cards will contain no duplicate cards, and will 
    therefore contain some hearts or spades, represented by H or S.  
    A valid list of 31 cards will therefore never be a valid 
    hexadecimal string.
    """
    cleanArgument = nonwhitespace(argument).upper()
    check_for_unrecognised_characters(cleanArgument)

Does this really help you? Can you get away with just checking for valid hex or valid cards?

    try:
        value = int(cleanArgument, 16)   # Gives error if not hex.
    except ValueError:
        print(convert_to_hex(cleanArgument))
    else:
        print(convert_to_cards(value))

def nonwhitespace(argument):
    """Return argument with all whitespace removed.

    This includes removing any single spaces within the string.
    """
    return "".join(argument.split())

That's not a very efficient way to do that. Instead, I'd suggest using a regex replace to find all the whitespace and remove it.

def check_for_unrecognised_characters(argument):
    """Raise an exception if a character is unrecognised.

A "check" function should probably return True or False to indicate the value checked. An exception should indicate that the function failed to accomplish what was asked. In terms of checking, its not failure to have that be check came up false.

    The only recognised characters in this context are hexadecimal 
    characters and card ranks and suits.
    """
    for i in argument:

i is bad name. Firstly, I'd suggest avoiding single letter variable names, they are hard to determine what they are. Also, i usually is taken to stand for index, and this isn't an index.

        if i not in recognisedCharacters:
            message = ("Character '" + i + 
                "' not recognised as part of card or hexadecimal.")
            raise UnrecognisedCharacterError(message)

I suggest using repr(i) to add the quotes. Also look at string formatting rather than concatenating strings.

def convert_to_hex(argument):
    check_number_of_characters(argument)
    listOfCards = [argument[i:i+2] for i in range(0, 62, 2)]
    check_if_cards(listOfCards)
    check_for_card_repetition(listOfCards)
    return hex_representation(listOfCards)

I'd suggest that you really want something like:

cards = string_to_cards(text_input) # throws exception if not valid cards
return cards_to_hex(cards)

Whereas this function mixes the idea of reading the card representation with it overall logic.

def convert_to_cards(value):
    check_hex_is_in_range(value)
    return corresponding_cards(value)

def check_number_of_characters(argument):

The function name is confusing, because it gives no hints about what character limit is going on.

    """Raise an exception if not exactly 31 cards."""
    length = len(argument)
    if length < 62:
        message = (
            "31 cards required, each 2 characters.\n"
            "62 characters required in total.\n"
            "Only " + str(length) + " nonwhitespace characters provided."
            )
        raise TooFewCardsError(message)
    if length > 62:
        message = (
            "31 cards required, each 2 characters.\n"
            "62 characters required in total.\n"
            "" + str(length) + " nonwhitespace characters provided."
            )
        raise TooManyCardsError(message)

def check_if_cards(listOfCards):
    """Raise an exception if not valid cards.

    Every card should be a rank character followed by a suit character.
    """
    for i in listOfCards:
        if i[0] not in cardRanks:
            message = (
                "'" + str(i) + "' is not a recognised card rank.\n"
                "A valid rank is a single character as follows:\n"
                "'A' (ace)\n"
                "'2' (two)\n"
                "'3' (three)\n"
                "'4' (four)\n"
                "'5' (five)\n"
                "'6' (six)\n"
                "'7' (seven)\n"
                "'8' (eight)\n"
                "'9' (nine)\n"
                "'T' (ten)\n"
                "'J' (jack)\n"
                "'Q' (queen)\n"
                "'K' (king)"
                )
            raise UnrecognisedCardRankError(message)
        if i[1] not in cardSuits:
            message = (
                "'" + str(i) + "' is not a recognised card suit.\n"
                "A valid suit is a single character as follows:\n"
                "'S' (spades)\n"
                "'H' (hearts)\n"
                "'D' (diamonds)\n"
                "'C' (clubs)"
                )
            raise UnrecognisedCardSuitError(message)

You don't really need to create so many exception classes. Also putting the user interface text in the exception is problematic.

def check_for_card_repetition(listOfCards):
    """Check that there are 31 unique pairs of characters.

    The list is already known to contain exactly 31 pairs.  Just check 
    that each is unique.
    """
    uniqueCards = set(listOfCards)
    if not len(uniqueCards) == 31:
        message = (
            "No two cards should be the same.\n"
            "Cards should be drawn from a single deck of 52 cards.\n"
            "Cards should be drawn without replacement."
            )
        raise DuplicatedCardsError(message)

def check_hex_is_in_range(value):
    """Check 0 <= value <= 52!/(52-31)! - 1

    As Python's arbitrary precision integers cannot be represented
    in hexadecimal as negative without using a minus sign, which has
    already been precluded, check only the upper limit.
    """
    if value > upperLimit:
        message = (
            "The hexadecimal value is too large to be represented by 31 cards.\n"
            "The maximum valid value is 52!/(52-31)! - 1\n"
            "In hexadecimal this maximum is\n"
            "114882682E46B11EADE9F57C1E3E0BBD47FFFFFFF"
            )
        raise HexValueTooLargeError(message)

def hex_representation(listOfCards):
    """Return a hexadecimal string defined by the 31 cards.

    The 52 cards in the full deck are numbered from 0 to 51.
    The order used here is defined by allCards.
    The 1st card in listOfCards therefore gives a number from 0 to 51.
    Remove this card from the deck so the deck is now numbered from 
    0 to 50.
    The 2nd card in listOfCards now gives a number from 0 to 50.
    Continue in the same way, to convert the 3rd card to a number 
    from 0 to 49, and so on.
    The final (31st) card will be converted to a number from 0 to 21.
    There is now a list of 31 numbers, each in a smaller range than 
    the last.
    Keep the 1st number as it is (multiply by 1).
    Multiply the 2nd number by 52.
    Multiply the 3rd number by 52 * 51.
    Multiply the 4th number by 52 * 51 * 50.
    Continue until all 31 numbers have been updated in this way.
    The required result is the sum of this list of 31 numbers.
    """
    listOfNumbers = []
    deck = allCards
    for card in listOfCards:
        number = deck.index(card)
        listOfNumbers.append(number)
        deck.remove(card)

    for n in range(31):
        listOfNumbers[n] *= factorial52 // factorial(52-n)
    result = sum(listOfNumbers)
    return hex(result)[2:]

def corresponding_cards(value):
    """Return a string of 31 cards representing the value.

    Divide the value by 52 making a note of quotient and remainder.
    The remainder will be a number from 0 to 51.
    Start a list of numbers with this remainder.
    Use the quotient to continue the process.
    Divide the quotient by 51 making a note of quotient and remainder.
    The remainder will be a number from 0 to 50.
    Append this remainder to the list of numbers.
    Use the new quotient to continue the process.
    Continue until the list contains 31 numbers.
    The 1st number will be from 0 to 51, and defines a card from the 
    full deck of 52 cards (in the order defined by allCards).
    The 2nd number will be from 0 to 50, and defines a card from the 
    remaining 51 cards.
    Continue in the same way to convert the remaining numbers to cards.
    """
    deck = allCards
    listOfNumbers = []
    listOfCards = []
    for i in range(31):
        divisor = 52 - i
        quotient = value // divisor
        remainder = value % divisor
        listOfNumbers.append(remainder)
        value = quotient
    for cardNumber in listOfNumbers:
        card = deck.pop(cardNumber)
        listOfCards.append(card)
    return " ".join(listOfCards)

# Handle the case where this program is called from the command line.
if __name__ == "__main__":
    import sys
    arguments = sys.argv
    if len(arguments) < 2:
        argument = request_input()
    else:
        argument = "".join(arguments[1:])
    process(argument)
\$\endgroup\$
  • \$\begingroup\$ Thanks very much for all the improvements. I use factorial52 in some of the later functions, so I can see now how this would benefit from being explicitly a constant in CAPS. Otherwise it just looks like a pointless variable for just that one line. It's great to have reasons for all the improvements so I can apply them elsewhere too. \$\endgroup\$ – trichoplax Feb 23 '14 at 0:25
  • \$\begingroup\$ I haven't used your suggestion of itertools.product as it gives me a different card order and tuples instead of strings of 2 characters. Instead I've replaced the nested for loops you highlighted with a one line list comprehension which hopefully looks less cluttered. \$\endgroup\$ – trichoplax Feb 23 '14 at 1:38

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.