Anyone want to critique this program?

Discussion in 'Python' started by John Salerno, Jul 3, 2011.

  1. John Salerno

    John Salerno Guest

    Just thought I'd post this in the event anyone has a few spare minutes
    and feels like tearing apart a fairly simple attempt to write a
    game. :)

    I'll paste the exercise I was working on first, although I think it
    was meant to be an exercise in how to use lists. I went way beyond
    that, so maybe my program is overly complicated. It works, though, so
    that's a plus! The main questions I had about it are at the bottom,
    after the code.

    ----------
    Snakes and Ladders can be a fun game for kids to play but you realize
    when you get older that the players actually have no choices at all.
    To illustrate just how little input the players have I want you to
    make a computer program that allows two players to play snakes and
    ladders on the board given.

    The rules of the game are simple. On each player's turn they roll one
    six sided die and move ahead that many squares. If they end their
    move at the base of a ladder then they automatically climb to the top
    of the ladder. If they end their move at the end of a snake they
    automatically slide down to its head. To win the game you must be the
    first player to land on the last square. If you are near the end and
    your roll would cause you to go past the end square you don't move for
    that turn.

    Your computerized version will look something like:
    Player 1 hit enter to roll

    You rolled: 3
    You are at spot 3
    Player 2 hit enter to roll

    You rolled: 6
    You climbed a ladder
    You are at spot 17
    although you can write this program without using lists, you should
    ask yourself how you can use lists to encode where the snakes and
    ladders are.
    -----------------
    (I changed snakes to chutes, and I didn't paste in the game board
    picture. Just trust that the dictionary of lists in the beginning of
    the program is accurate, and that each set of numbers represents first
    the space you land on, and second the space you slide or climb to,
    e.g. [30, 35] means you landed on 30, and as a result of landing on a
    ladder space, you climbed up to space 35.)

    -------------------------------
    import random

    game_information = '***Chutes and Ladders***\nUp to four (4) players
    may play.\n'\
    'There are 90 spaces on the board. '\
    'The player to reach space 90 first wins.'
    separator = '-' * 20
    spaces = [None] * 90
    c_l_spaces = {r'slid down a chute \\': [[14, 3], [20, 15], [39, 33],
    [66, 53],
    [69, 58], [79, 67], [84, 71], [88,
    36]],
    'climbed up a ladder |=|': [[6, 17], [24, 26], [30, 44],
    [49, 62], [82, 86]]}

    class Player:

    def __init__(self, number):
    self.number = number
    self.space = 0

    def move(self, roll):
    global spaces
    if (self.space + roll) > 90:
    return (1, 'Your roll would move you off the game board.
    '\
    'You remain at space {0}.'.format(self.space))
    elif (self.space + roll) == 90:
    return (0, 'You moved to the last space. You won the
    game!')
    else:
    self.space += roll
    try:
    space_type = spaces[self.space - 1][0]
    self.space = spaces[self.space - 1][1]
    except TypeError:
    return (1, 'You moved to space
    {0}.'.format(self.space))
    else:
    return (1, 'You {0} to space {1}!'.format(space_type,
    self.space))

    def roll_die():
    roll = random.randint(1, 6)
    print('You rolled {0}.'.format(roll))
    return roll

    def populate_game_board():
    global spaces
    for key, values in c_l_spaces.items():
    for space in values:
    spaces[space[0] - 1] = [key, space[1]]

    def initialize_players():
    while True:
    try:
    num_players = int(input('Enter the number of players (0 to
    exit): '))
    except ValueError:
    print('You have entered an invalid response.\n')
    else:
    if num_players == 0:
    print('You have quit the game.')
    return
    elif 1 <= num_players <= 4:
    break
    elif (num_players < 0) or (num_players > 4):
    print('You have entered an invalid number of players.
    \n')

    players = []
    for num in range(num_players):
    players.append(Player(num + 1))

    return players

    def start_game(players):
    game = 1
    while game:
    for player in players:
    print('\n***Player {0}***'.format(player.number))
    print('You are currently on space
    {0}.'.format(player.space))
    game_update = player.move(roll_die())
    print(game_update[1])
    print(separator, end='')
    if game_update[0] == 0:
    game = 0
    break
    if game:
    input('\nPress Enter for the next turn.')

    def initialize_game():
    global game_information
    print(game_information)
    players = initialize_players()
    if players:
    populate_game_board()
    start_game(players)

    if __name__ == '__main__':
    initialize_game()
    --------------------------

    My questions:

    1. Are the functions I made necessary? Do they do enough or too much?
    2. Is there a better way to implement the players than as a class?
    3. Is there a better way to handle all the print calls that the
    program makes?
    4. Are my try/except blocks used appropriately?

    Any other comments would be appreciated too!
    Thanks!
     
    John Salerno, Jul 3, 2011
    #1
    1. Advertising

  2. On Sun, Jul 3, 2011 at 12:19 PM, John Salerno <> wrote:
    > Just thought I'd post this in the event anyone has a few spare minutes
    > and feels like tearing apart a fairly simple attempt to write a
    > game. :)


    Sure! You seem to comprehend the basics, which is an order of
    magnitude better than some people I've tried to help...

    > game_information = '***Chutes and Ladders***\nUp to four (4) players
    > may play.\n'\
    >                   'There are 90 spaces on the board. '\
    >                   'The player to reach space 90 first wins.'


    I'd do this with a triple-quoted string - it'll simply go across multiple lines:
    game_information = """***Chutes and Ladders***
    Up to four (4) players may play.
    There are 90 spaces on the board.
    The player to reach space 90 first wins."""

    > c_l_spaces = {r'slid down a chute \\': [[14, 3], [20, 15], [39, 33],
    > [66, 53],
    >                                    [69, 58], [79, 67], [84, 71], [88,
    > 36]],
    >              'climbed up a ladder |=|': [[6, 17], [24, 26], [30, 44],
    > [49, 62], [82, 86]]}


    It strikes me that this plus populate_game_board() is a little
    redundant; you could simply have the target dictionary as a literal:

    spaces={14:3, 20:15, ........ 6:17, 24:26}

    You can get the description "climbed up a ladder" or "slid down a
    chute" by seeing if spaces[space] is more or less than space.

    >            try:
    >                space_type = spaces[self.space - 1][0]
    >                self.space = spaces[self.space - 1][1]
    >            except TypeError:
    >                return (1, 'You moved to space
    > {0}.'.format(self.space))
    >            else:
    >                return (1, 'You {0} to space {1}!'.format(space_type,
    > self.space))


    It strikes me as odd to use try/except/else for what's
    non-exceptional. I'd either use a regular if block, or possibly
    something completely different - like having a list like this:

    spaces=list(range(91)) # get a list [0, 1, 2, 3... 90]

    And then set the entries that have chutes/ladders, possibly from your
    original dict of lists (or something like it):

    for space in values:
    spaces[space[0]] = space[1]

    Then to see where you end up, just go:
    try:
    space=spaces[space]
    except ValueError:
    print("That would take you past the end of the board!")

    In this instance, except is being used for the exceptional condition,
    the case where you blow past the edge of the board - rather than the
    more normal situation of simply not hitting a chute/ladder.

    >    players = []
    >    for num in range(num_players):
    >        players.append(Player(num + 1))


    Generally, anything that looks like this can become a list
    comprehension. It'll be faster (which won't matter here), and briefer.

    players = [Player(num+1) for num in range(num_players)]

    > def start_game(players):
    >
    > def initialize_game():
    >        start_game(players)
    >
    > if __name__ == '__main__':
    >    initialize_game()


    start_game() would be more accurately called play_game(), since it
    doesn't return till the game's over. And it's a little odd that
    initialize_game() doesn't return till the game's over; I'd be inclined
    to have initialize_game() return after initializing, and then have the
    main routine subsequently call start_game() / play_game(). Just a
    minor naming issue!

    > 1. Are the functions I made necessary? Do they do enough or too much?


    For the most part, I would say your functions are fine.

    > 2. Is there a better way to implement the players than as a class?


    Better way? Hard to know. There are many ways things can be done, but
    the way you've chosen is as good as any. Certainly it's good enough
    for the task you're doing.

    > 3. Is there a better way to handle all the print calls that the
    > program makes?


    Again, I think it's fine. There's a general philosophy of separating
    "guts" from "interface" (which would mean isolating all the print
    statements from the game logic of moving pieces around), but in
    something this small, the only reason to do that is for the sake of
    practice. There's not much to be gained in small programs from fifty
    levels of indirection.

    > 4. Are my try/except blocks used appropriately?


    This is the one place where I'd recommend change. Use try/except to
    catch exceptional situations, not normalities. If your code is going
    through the except block most of the time, there's probably something
    wrong. Note I don't say there IS something wrong; it's an example of
    code smell, and can be correct.

    Your code has much in its favour. I've been wordy in suggestions but
    that doesn't mean you have bad code! :)

    Chris Angelico
     
    Chris Angelico, Jul 3, 2011
    #2
    1. Advertising

  3. John Salerno

    John Salerno Guest

    On Jul 2, 10:02 pm, Chris Angelico <> wrote:

    > > game_information = '***Chutes and Ladders***\nUp to four (4) players
    > > may play.\n'\
    > >                   'There are 90 spaces on the board. '\
    > >                   'The player to reach space 90 firstwins.'

    >
    > I'd do this with a triple-quoted string - it'll simply go across multiplelines:
    > game_information = """***Chutes and Ladders***
    > Up to four (4) players may play.
    > There are 90 spaces on the board.
    > The player to reach space 90 first wins."""


    Yeah, I considered that, but I just hate the way it looks when the
    line wraps around to the left margin. I wanted to line it all up under
    the opening quotation mark. The wrapping may not be as much of an
    issue when assigning a variable like this, but I especially don't like
    triple-quoted strings that wrap around inside function definitions.
    That seems to completely throw off the indentation. Do people still
    use triple-quotes in that situation?

    > > c_l_spaces = {r'slid down a chute \\': [[14, 3], [20, 15], [39, 33],
    > > [66, 53],
    > >                                    [69, 58], [79, 67], [84, 71], [88,
    > > 36]],
    > >              'climbed up a ladder |=|': [[6, 17], [24, 26], [30, 44],
    > > [49, 62], [82, 86]]}

    >
    > It strikes me that this plus populate_game_board() is a little
    > redundant; you could simply have the target dictionary as a literal:
    >
    > spaces={14:3, 20:15, ........ 6:17, 24:26}
    >
    > You can get the description "climbed up a ladder" or "slid down a
    > chute" by seeing if spaces[space] is more or less than space.


    Hmm, interesting. I'm not quite sure how I'd implement that yet, but
    the "14:3" structure seems cleaner (although I admit at first it
    looked weird, because I'm used to seeing strings as dictionary
    keywords!).

    > >            try:
    > >                space_type = spaces[self.space - 1][0]
    > >                self.space = spaces[self.space - 1][1]
    > >            except TypeError:
    > >                return (1, 'You moved to space
    > > {0}.'.format(self.space))
    > >            else:
    > >                return (1, 'You {0} to space {1}!'.format(space_type,
    > > self.space))

    >
    > It strikes me as odd to use try/except/else for what's
    > non-exceptional. I'd either use a regular if block, or possibly
    > something completely different - like having a list like this:
    >
    > spaces=list(range(91)) # get a list [0, 1, 2, 3... 90]
    >
    > And then set the entries that have chutes/ladders, possibly from your
    > original dict of lists (or something like it):
    >
    > for space in values:
    >   spaces[space[0]] = space[1]
    >
    > Then to see where you end up, just go:
    > try:
    >   space=spaces[space]
    > except ValueError:
    >   print("That would take you past the end of the board!")
    >
    > In this instance, except is being used for the exceptional condition,
    > the case where you blow past the edge of the board - rather than the
    > more normal situation of simply not hitting a chute/ladder.


    Originally I didn't have the try/except at all, I had nested if
    statements that seemed to be getting out of control. Then I realized
    if I just attempted to index the list and handle the exception, rather
    than check first if indexing the list was allowed, the code came out
    cleaner looking. But I agree with you, my "exception" is actually the
    more frequent occurrence, so I'm going to change that.

    > >    players = []
    > >    for num in range(num_players):
    > >        players.append(Player(num + 1))

    >
    > Generally, anything that looks like this can become a list
    > comprehension. It'll be faster (which won't matter here), and briefer.
    >
    > players = [Player(num+1) for num in range(num_players)]


    Now this is the kind of tip I love. Something like list comprehensions
    just didn't even occur to me, so I'm definitely going to change that.

    > > def start_game(players):

    >
    > > def initialize_game():
    > >        start_game(players)

    >
    > > if __name__ == '__main__':
    > >    initialize_game()

    >
    > start_game() would be more accurately called play_game(), since it
    > doesn't return till the game's over. And it's a little odd that
    > initialize_game() doesn't return till the game's over; I'd be inclined
    > to have initialize_game() return after initializing, and then have the
    > main routine subsequently call start_game() / play_game(). Just a
    > minor naming issue!


    Minor or not, it makes sense. I'm picky about things like that too, so
    now that you've pointed it out, I'm compelled to change the names so
    they make sense! :)

    > > 2. Is there a better way to implement the players than as a class?

    >
    > Better way? Hard to know. There are many ways things can be done, but
    > the way you've chosen is as good as any. Certainly it's good enough
    > for the task you're doing.


    Yeah, I don't need to know 10 different ways to do things. Mainly I
    asked this question because the original exercise seemed to focus on
    using lists to write the game, and I just couldn't think of an
    efficient way to use a list to store the player attributes like what
    space they were on. It just seemed a natural candidate for a class
    attribute.

    > > 3. Is there a better way to handle all the print calls that the
    > > program makes?

    >
    > Again, I think it's fine. There's a general philosophy of separating
    > "guts" from "interface" (which would mean isolating all the print
    > statements from the game logic of moving pieces around), but in
    > something this small, the only reason to do that is for the sake of
    > practice. There's not much to be gained in small programs from fifty
    > levels of indirection.


    Heh, then you should have seen the original version I wrote! All the
    print calls were *inside* the "move" method, but it felt strange to
    have the move method do the printing directly, so I changed it to a
    return statement and handled the printing elsewhere. Although I still
    wonder if I could abstract the print calls even further. The presence
    of all that text just seems like it begs for a clean way to handle it.

    > Your code has much in its favour. I've been wordy in suggestions but
    > that doesn't mean you have bad code! :)


    Thanks a lot for the tips! I'm going to go back over the whole thing
    and rework some of it. And I'm only doing this for the sake of
    learning, so even the small tips help me to think more like a
    programmer. :)
     
    John Salerno, Jul 3, 2011
    #3
  4. On Sun, Jul 3, 2011 at 1:41 PM, John Salerno <> wrote:
    > Yeah, I considered that, but I just hate the way it looks when the
    > line wraps around to the left margin. I wanted to line it all up under
    > the opening quotation mark. The wrapping may not be as much of an
    > issue when assigning a variable like this, but I especially don't like
    > triple-quoted strings that wrap around inside function definitions.
    > That seems to completely throw off the indentation. Do people still
    > use triple-quotes in that situation?


    Jury's out on that one. You can either back-tab it to the left (looks
    ugly), or indent it and then clean it up in code (IS ugly). Up to you
    to figure out what's best for your code, and if you want to indent it,
    your current way is quite possibly the cleanest.

    > Minor or not, it makes sense. I'm picky about things like that too, so
    > now that you've pointed it out, I'm compelled to change the names so
    > they make sense! :)


    :) Names tend to stay the same when the functions they're attached to
    grow and shift. Sometimes you end up with these great warts in huge
    production code... it's sometimes just too much work to change things.

    > Thanks a lot for the tips! I'm going to go back over the whole thing
    > and rework some of it. And I'm only doing this for the sake of
    > learning, so even the small tips help me to think more like a
    > programmer. :)


    Learning's good! And Python's an excellent language for the purpose.
    Code quickly, invoke quickly (no compilation stage), and see the
    results of the work.

    ChrisA
     
    Chris Angelico, Jul 3, 2011
    #4
  5. John Salerno wrote:

    > On Jul 2, 10:02 pm, Chris Angelico <> wrote:
    >> I'd do this with a triple-quoted string - it'll simply go across
    >> multiple lines: game_information = """***Chutes and Ladders***
    >> Up to four (4) players may play.
    >> There are 90 spaces on the board.
    >> The player to reach space 90 first wins."""

    >
    > Yeah, I considered that, but I just hate the way it looks when the
    > line wraps around to the left margin. I wanted to line it all up
    > under the opening quotation mark. The wrapping may not be as much
    > of an issue when assigning a variable like this, but I especially
    > don't like triple-quoted strings that wrap around inside function
    > definitions. That seems to completely throw off the indentation. Do
    > people still use triple-quotes in that situation?


    I do, because I use an editor that intelligently indents wrapped
    text to the same indent level as the beginning of the line, instead of
    wrapping it all the way back to the margin.

    --
    --OKB (not okblacke)
    Brendan Barnwell
    "Do not follow where the path may lead. Go, instead, where there is
    no path, and leave a trail."
    --author unknown
     
    OKB (not okblacke), Jul 3, 2011
    #5
  6. John Salerno

    John Salerno Guest

    On Jul 3, 1:06 pm, "OKB (not okblacke)"
    <> wrote:

    > > Yeah, I considered that, but I just hate the way it looks when the
    > > line wraps around to the left margin. I wanted to line it all up
    > > under the opening quotation mark. The wrapping may not be as much
    > > of an issue when assigning a variable like this, but I especially
    > > don't like triple-quoted strings that wrap around inside function
    > > definitions. That seems to completely throw off the indentation. Do
    > > people still use triple-quotes in that situation?

    >
    >         I do, because I use an editor that intelligently indents wrapped
    > text to the same indent level as the beginning of the line, instead of
    > wrapping it all the way back to the margin.


    But isn't wrapped text something different than text that is purposely
    split across multiple lines with a newline character? That's usually
    the case when I need to split up a long string.
     
    John Salerno, Jul 4, 2011
    #6
  7. John Salerno wrote:

    > On Jul 3, 1:06 pm, "OKB (not okblacke)"
    > <> wrote:
    >
    >> > Yeah, I considered that, but I just hate the way it looks when the
    >> > line wraps around to the left margin. I wanted to line it all up
    >> > under the opening quotation mark. The wrapping may not be as much
    >> > of an issue when assigning a variable like this, but I especially
    >> > don't like triple-quoted strings that wrap around inside function
    >> > definitions. That seems to completely throw off the indentation. Do
    >> > people still use triple-quotes in that situation?

    >>
    >>         I do, because I use an editor that intelligently indents
    >> wrapped text to the same indent level as the beginning of the line,
    >> instead of wrapping it all the way back to the margin.

    >
    > But isn't wrapped text something different than text that is purposely
    > split across multiple lines with a newline character? That's usually
    > the case when I need to split up a long string.


    Well, what I'm saying is I use an editor that lets me make the
    lines as long as I want, and it still wraps them right, so I never
    explicitly hit enter to break a line except at the end of a string (or
    paragraph).

    --
    --OKB (not okblacke)
    Brendan Barnwell
    "Do not follow where the path may lead. Go, instead, where there is
    no path, and leave a trail."
    --author unknown
     
    OKB (not okblacke), Jul 4, 2011
    #7
  8. On Tue, Jul 5, 2011 at 2:37 AM, OKB (not okblacke)
    <> wrote:
    >        Well, what I'm saying is I use an editor that lets me makethe
    > lines as long as I want, and it still wraps them right, so I never
    > explicitly hit enter to break a line except at the end of a string (or
    > paragraph).


    In this instance, I believe the OP was paragraphing his text. Is there
    a convenient way to do that in a triple-quoted string?

    My personal inclination would be to simply back-tab it. It looks ugly,
    but at least it works, and doesn't require a run-time re-parse. The
    run-time translation is good for docstrings, though (which are
    syntactically the same thing as this situation).

    ChrisA
     
    Chris Angelico, Jul 4, 2011
    #8
  9. On Tue, Jul 5, 2011 at 10:03 AM, Ben Finney <> wrote:
    > Chris Angelico <> writes:
    >
    >> In this instance, I believe the OP was paragraphing his text.

    >
    > What is “paragraphing”?


    If you look at the original code, you'll see embedded newlines used to
    create multiple paragraphs. Hence, paragraphing as opposed to simply
    wrapping in order to keep his source lines <80 chars.

    >> My personal inclination would be to simply back-tab it. It looks ugly,
    >> but at least it works, and doesn't require a run-time re-parse.

    >
    > Readability counts. Why is “doesn't require a run-time re-parse” more
    > valuable than readability?


    Readability definitely counts. But having a string literal require an
    extra call to make it what you want also costs readability. It's like
    pushing your literals off to an i18n file - what you gain in
    flexibility, you lose in simplicity.

    ChrisA
     
    Chris Angelico, Jul 5, 2011
    #9
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. G Patel

    Please critique my code/program

    G Patel, Feb 27, 2005, in forum: C Programming
    Replies:
    8
    Views:
    338
    Gregory Pietsch
    Mar 2, 2005
  2. BTM
    Replies:
    8
    Views:
    417
    Jonathan N. Little
    Jun 4, 2007
  3. critique my program!

    , Aug 11, 2007, in forum: C Programming
    Replies:
    10
    Views:
    588
    Ian Collins
    Aug 12, 2007
  4. cs

    Request critique of first program

    cs, Sep 2, 2007, in forum: C Programming
    Replies:
    3
    Views:
    281
  5. cs

    Request critique of first program

    cs, Sep 2, 2007, in forum: C Programming
    Replies:
    14
    Views:
    493
    Keith Thompson
    Sep 4, 2007
Loading...

Share This Page