Anyone want to critique this program?

[John Salerno]

On Jul 2, 10:02 pm, Chris Angelico <ros... at gmail.com> 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 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."""

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. :)