Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am teaching computing to an AS class and this was their assignment:

Bush Wanderer

Task 1

Create a 5x5 grid of hashes(#) where the player is an x on the board

Prompt the player to move in either N, E, S or W direction and then reprint the board to show the new position.

Example:

x # # # #
# # # # # 
# # # # #
# # # # #
# # # # #
Which way would you like to move?(N,E,S or W) 

Task 2

  • Ensure the player does not move off of the board

  • Place a hidden chest somewhere on the board at random

  • End the game when the chest is found

  • Tell the player how many moves it took to find the chest

  • Add a visible (but blind) dragon that can teleport around the board at random and will eat you and end your game if it gets you before
    you get the chest

This was my exemplar solution at the end and I wanted to get some feedback on what the professional community thought of it. How could this be improved?

import random

grid = 5

moves = 0

x = 0
y = 0

cx = random.randint(0,(grid-1))
cy = random.randint(0,(grid-1))

while(True):

  dx = random.randint(0,(grid-1))
  dy = random.randint(0,(grid-1))

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:  
        for i in range(grid):
          print('# ', end="")
        print()

  if dx == x and dy == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  elif cx == x and cy == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

  m = input('Which way would you like to move?(N,E,S or W)').lower()

  if m == 'n' and y > 0:
      y = y-1
  elif m == 'e' and x < (grid-1):
      x = x+1
  elif m == 's' and y < (grid-1):
      y = y+1
  elif m == 'w' and x >0:
      x = x-1
  else:
      print('Invalid move')

  moves = moves + 1
share|improve this question
2  
"AS" Class? what does the shorthand stand for? – Pimgd Feb 26 at 15:05
5  
Advanced Subsidiary - Year 12 in the UK (16/17 year olds) – Toby Adams Feb 26 at 15:07
    
@TobyAdams thanks for the accept, did I get everything you were looking for? It's not really an "industry standards" view, and more tailored towards the idea that you're going to be teaching this stuff. – Pimgd Feb 26 at 15:46
    
@Pimgd your feedback in unreal, thank you. I never expected that level of response in that kind of time frame. I'm going away now to re-think this and incorporate some of the things you have suggested. – Toby Adams Feb 26 at 16:00
2  
Your students will follow your example, and avoid the use of comments and meaningful variable names. A good way to encourage their use is to have the students then trade code and modify it to add a new feature. – rom016 Feb 27 at 16:32
up vote 34 down vote accepted

Because the code you write is to be used for teaching, the importance is not on performance, but on correctness and readability and understandability (comphrensiveness?).

So basically...

  • Add comments! Explain each step, so that the kids are gonna see the code, think "what does it do", and they can read it right there!

  • Don't use dx and dy, use more explanatory variables!

Let's combine them:

  if dx == x and dy == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  elif cx == x and cy == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

This is hard to understand. dx and dy were hard to grasp at first - when doing games programming, usually dx and dy are "delta-x" and "delta-y", to calculate object velocity.

But it's dragon x y and chest x y here!

So just call it that.

  if dragon_x == x and dragon_y == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  elif chest_x == x and chest_y == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

Next, I'd usually tell you to split this up into functions. But maybe these kids don't know functions yet, and it's remarkably frustrating to be shown "the solution" when "the solution" consists of "this magic trick I didn't teach you yet, haha".

Use comments instead, and be sure to teach them what a comment is and what its for. If you have to review homework, a comment might be all you get to understand their code.

  #check for loss by comparing dragon and player location
  if dragon_x == x and dragon_y == y:
    if moves == 1:
      print('You died in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You died in', moves,'moves!')
    break
  #else, check for victory by comparing chest and player location
  elif chest_x == x and chest_y == y:
    if moves == 1:
      print('You won in', moves,'move!')
    elif moves > 1 or moves == 0:
      print('You won in', moves,'moves!')
    break

Note that function names make good comments. Instead of checkForVictory() we just put #check for victory. I recommend you start comments with why, and then with how. How-comments should eventually be removed (you can read the code), and why-comments should stay, but when learning how to program, having the explanation next to the code just helps. It also promotes micro problem-solution flow (in my opinion), which is what you need later if you want to split up code into smaller chunks or problems.

And this loop here...

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:  
        for i in range(grid):
          print('# ', end="")
        print()

Is mighty confusing. There is no easily discernible pattern to it. If you had to explain the thought process that went into each step, what would you say? (Teacher, I don't understand, how did you create this?)

Here's mine:

per tile:
  print dragon if dragon is on tile
  print player if player is on tile
  print empty tile if nobody is on tile

Those are the requirements, next up is realizing that if we have a grid, we ought to do it by row and columns:

for each row
  for each column
    if the dragon is on that tile
     print dragon
    else if the player is on that tile
     print player
    else
     print empty tile
  print new line, to go to next row

And then translate to python (untested)

for i in range(grid)
  for u in range(grid)
    if i == dragon_y and u == dragon_x:
     print('@ ', end='')
    elif i == player_y and u == player_x:
     print('x ', end='')
    else:
     print('# ', end='')
  print()

Another reason for this is that if you were to get a question like, "how do we add mountains, where the dragon can wander, but the player can't?", you'd be stumped. Or at least I would be, if I had to add mountains into your display code. It's all about clever tricks with multiplying strings. You'd have to alter ALL your cases. Or if there were two dragons, because, you know, hard mode. Again, it would take you a major overhaul.

I'd just make a simple change:

for each row
  for each column
    if there is a dragon on that tile
     print dragon
    else if the player is on that tile
     print player
    else if that tile is a mountain
     print mountain
    else
     print empty tile
  print new line, to go to next row

and it supports mountains and multiple dragons!

Possibly the best thing you could do is take another look at your code, and remove the python element from it. Start in English, and convert that to python. I bet that's what the kids will be doing.

Using an approach like this, the code is going to be a lot shorter and easy to understand. Yes, it's slower by doing all the checks all the time. But for these kids, it's probably going to be more important that they understand the code, than that it runs three times as fast. Computers these days are forgiving, and you can always teach them better optimizations later.

Plus, if they were to shy away from programming because the code is too complex, that'd be a darn shame.

And get comfortable with your code before the lesson. It would be best if you could do live editing in the classroom.


As for industry standards...

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:  
        for i in range(grid):
          print('# ', end="")
        print()

Extract to printGrid.

  m = input('Which way would you like to move?(N,E,S or W)').lower()

  if m == 'n' and y > 0:
      y = y-1
  elif m == 'e' and x < (grid-1):
      x = x+1
  elif m == 's' and y < (grid-1):
      y = y+1
  elif m == 'w' and x >0:
      x = x-1
  else:
      print('Invalid move')

  moves = moves + 1

Extract to pickMove, and use a while loop to make sure that the move counter (moves) doesn't go up if you make a bad move.

Encapsulate conditionals by putting checks like dy == y and dx == x into either a function isDragonEatingPlayer (naming is hard) or just a temporary variable of some sorts.

share|improve this answer
4  
Good point suggesting to split it up into functions. Shying away from functions seems to be a "common theme" when it comes to teaching programming, which is entirely unreasonable, because it's part of the "main idea" of hiding complex things behind simple names (abstraction). The students I know that are using functions (even if sometimes too excessively) are generally performing a lot better. – Daniel Jour Feb 27 at 8:32
    
@DanielJour. Is there something like using to many functions, in case we don't care about performance? – magu_ Apr 26 at 15:28
    
@magu_ I think so: A function should do one job. If it does more, split it up, if it does less, get rid of it. – Daniel Jour Apr 29 at 7:00

My main criticism is that the code is hard to follow because the variable names are cryptic and the logic is very if-else heavy.

I understand your desire to avoid overwhelming your students with language features, but at some point the code becomes un-Pythonic and more complicated without certain features. One feature that would help a lot is the tuple, because x, y, cx, cy, dx, dy are cryptic and cumbersome. This would be nicer:

from random import randint

GRID_SIZE = 5

your_xy = (0, 0)
chest_xy = (randint(0, GRID_SIZE-1), randint(0, GRID_SIZE-1))

Unfortunately, using tuples makes constructing the next coordinate a bit uglier, especially if you don't use a namedtuple or this trick to add coordinates.


The board-printing routine is the nastiest portion of the program:

  for i in range(grid):
      if i == dy and i == y:
        for u in range(grid):
          if u == dx:
            print('@ ', end='')
          elif u == x:
            print('x ', end='')
          else:
            print('# ', end='')
        print()
      elif i == y:
          print('# '*x, end="")
          print('x ', end="")
          print('# '*((grid-x)-1))
      elif i == dy:
          print('# '*dx, end="")
          print('@ ', end="")
          print('# '*((grid-dx)-1))
      else:  
        for i in range(grid):
          print('# ', end="")
        print()

It would be a lot simpler if you didn't try to use the * operator:

for y in range(GRID_SIZE):          # Note improved variable naming
    for x in range(GRID_SIZE):
        if (x, y) == dragon_xy:
            print('@ ', end='')
        elif (x, y) == your_xy:
            print('x ', end='')
        else:
            print('# ', end='')
    print()

This is an un-Pythonic way to write a counting loop:

while True:
    …
    moves = moves + 1

Counting loops are nearly always better written using some kind of range() or enumerate() or itertools. In this case, itertools.count() would be appropriate.

Note that you increment the count even when a move is invalid. I don't know if that is intentional.


There are also a few simple but important points.

By PEP 8, indentation should be four spaces. This is an important convention in Python, since indentation is significant.

As you may have noticed above, GRID_SIZE would be more appropriate than grid.

It would be a good idea to encourage some commenting, especially for code that was this cryptic. In particular, docstrings are an important habit to develop.


Suggested implementation

from itertools import count
from random import randint

"""
Bush wanderer: a game in which you wander on a grid until you win by finding a
hidden treasure chest or lose by being on the same square as a randomly moving
dragon.
"""

GRID_SIZE = 5

your_xy = (0, 0)
chest_xy = (randint(0, GRID_SIZE-1), randint(0, GRID_SIZE-1))

for moves in count():
    dragon_xy = (randint(0, GRID_SIZE-1), randint(0, GRID_SIZE-1))

    # Print board
    for y in range(GRID_SIZE):
        for x in range(GRID_SIZE):
            if (x, y) == dragon_xy:
                print('@ ', end='')
            elif (x, y) == your_xy:
                print('x ', end='')
            else:
                print('# ', end='')
        print()

    # Possible game end
    if dragon_xy == your_xy:
        if moves == 1:
            print('You died in {} move!'.format(moves))
        else:
            print('You died in {} moves!'.format(moves))
        break
    elif chest_xy == your_xy:
        if moves == 1:
            print('You won in {} move!'.format(moves))
        else:
            print('You won in {} moves!'.format(moves))
        break

    # Next move
    m = input('Which way would you like to move (N,E,S or W)? ').lower()
    if m == 'n' and your_xy[1] > 0:
        your_xy = (your_xy[0], your_xy[1] - 1)
    elif m == 'e' and your_xy[0] < GRID_SIZE-1:
        your_xy = (your_xy[0] + 1, your_xy[1])
    elif m == 's' and your_xy[1] < GRID_SIZE-1:
        your_xy = (your_xy[0], your_xy[1] + 1)
    elif m == 'w' and your_xy[0] > 0:
        your_xy = (your_xy[0] - 1, your_xy[1])
    else:
        print('Invalid move')
share|improve this answer
1  
Definately the most readable answer. My only gripe is using _xy instead of _position or _pos, but most people would consider that six of one and half a dozen of the other. – Pharap Feb 27 at 1:46
2  
@Pharap I've used _xy as a mnemonic for the tuple's contents, because a tuple can be mysterious. I'm not normally a fan of such suffixes. If I were writing this code for myself, I'd drop the suffix altogether and use other language features such as namedtuples or functions to make the code self-documenting. – 200_success Feb 27 at 2:38
    
@200_success Why is it simpler not to use the * operator when from here this is much easier both to read and understand? – Kieran Wootten Feb 29 at 14:01
    
Very nice answer. One nitpick, shouldn't the docstring come before the imports? – magu_ Apr 26 at 15:30

A list of guidelines to follow

  • Break out 'components' into classes
  • I find for non-ui based games, one should still think of it as if there was a ui, so break out the game logic from drawing the game board
  • Avoid nested conditionals
  • Make your python programs work as a module and executable (utilize if if __name__ == '__main__' so that when using the interpreter, you can import your module and mess around with the specific components and test variable adjustments
  • utilize verbs in naming functions
  • name variables full names, don't get lazy on that, code should read like a book and be self documenting
  • from a performance standpoint, you should avoid successive print statements, like when printing the board, opting to instead build your string up and use one print statement to dump out to the console - not a big deal for teaching, but it's best practice whether it's writing to a file or printing to console, to limit those interactions
  • avoid magic numbers and text, for example, you can see I defined a marker for the board, player, dragon, and kill/death
  • stick to consistent variable names ( don't use loc_x and location_x, stick with one or the other )
  • moving the movements into a dictionary allowed me to get rid of a ton of if/elif statements, and made the code much easier to read
  • very much personal opinion - line by line documentation is a sign of bad coding, in which the variables and/or functions are not named properly - You should be able to limit commenting to a brief description as a function docstring - I find code much easier to read without having to skip every other line that's a comment.
  • if you do feel compelled to inline comment a conditional block, I suggest always making the comment INSIDE the block vs above, this prevents cut and paste errors where one may leave the comment behind, and now it's left in the code, describing the wrong thing

ex: everything with a # i would consider not needed

def update_location(self, loc_x, loc_y):
    """Updates the players x and y coordinates, verifies they are not out of bounds prior to updating"""
    if self.range check(loc_x, loc_y):
        #  we have succeeded on the range check
        #  set the x-coordinate
        self.loc_x = loc_x
        #  set the y-coordinate
        self.loc_y = loc_y

Given the above list, you can probably find a few more areas that can be improved in the refactored code. One would be to remove these subtractions self.grid_size-1: and in the Game __init__ function, define self.max_east=self.grid_size-1, so that when we red the code, we understand the comparison -- I very much dislike doing math inside a boolean operation. It makes debugging harder and reduces code clarity. Feel free to make the change in the refactored code

Refactored Code:

import random

class Grid(object):
  def __init__(self, size = 5, board_marker = '#', player_marker = 'X', dragon_marker = 'D'):
    self.size             = size
    self.board_marker     = board_marker
    self.player_marker    = player_marker
    self.dragon_marker    = dragon_marker
    self.death_marker     = 'K'

  def update_grid(self, player, dragon):
    dragon_marker = self.death_marker if player.loc_x == dragon.loc_x and player.loc_y == dragon.loc_y else self.dragon_marker
    grid          = [ x[:] for x in [[self.board_marker]*self.size] * self.size ]
    grid[player.loc_y][player.loc_x]  = self.player_marker
    grid[dragon.loc_y][dragon.loc_x]  = dragon_marker
    return "\n%s\n" % '\n'.join([' '.join(row) for row in grid ])

class Player(object):
  def __init__(self, loc_x = 0, loc_y = 0):
    self.loc_x = loc_x
    self.loc_y = loc_y

class Treasure(object):
  def __init__(self, grid_size = 5):
    self.loc_x = random.randint(0,(grid_size-1))
    self.loc_y = random.randint(0,(grid_size-1))

class Dragon(object):
  def __init__(self, grid_size = 5):
    self.loc_x = random.randint(0,(grid_size-1))
    self.loc_y = random.randint(0,(grid_size-1))
    self.grid_size = grid_size

  def update_location(self):
    self.loc_x = random.randint(0,(self.grid_size-1))
    self.loc_y = random.randint(0,(self.grid_size-1))

class Game(object):
  def __init__(self, grid_size = 5):
    self.moves      = 0
    self.grid_size  = grid_size
    self.player     = Player()
    self.treasure   = Treasure()
    self.grid       = Grid()
    self.dragon     = Dragon()
    self.directions = []
    self.movement   = {
                        'n':( 0, -1),
                        's':( 0,  1),
                        'e':( 1,  0),
                        'w':(-1,  0),
                      }

  def run(self):
    while True:
      print "Dragon wanders about..."
      self.dragon.update_location()
      print self.grid.update_grid(self.player, self.dragon)

      if self.check_dragon():
        print "You've been eaten by the dragon in %d moves" % self.moves
        break

      self.move_player()
      print self.grid.update_grid(self.player, self.dragon)
      if self.check_dragon():
        print "Well that was dumb, you walked right into the dragon in %d moves" % self.moves
        break
      if self.check_treasure():
        print "You found the treasure in %d moves" % self.moves
        break

  def move_player(self):
    self.update_available_directions()
    while True:
      m = raw_input('Which way would you like to move? [ %s ]' % ', '.join(self.directions)).lower()
      if m in self.directions:
        break
      else:
        print "Invalid movement direction"
    pdx, pdy            = self.movement[m]
    self.player.loc_x  += pdx
    self.player.loc_y  += pdy
    self.moves         += 1

  def update_available_directions(self):
    self.directions = []
    if self.player.loc_x > 0:
      self.directions.append('w')
    if self.player.loc_x < self.grid_size-1:
      self.directions.append('e')
    if self.player.loc_y > 0:
      self.directions.append('n')
    if self.player.loc_y < self.grid_size-1:
      self.directions.append('s')

  def check_dragon(self):
    return self.player.loc_x == self.dragon.loc_x and self.player.loc_y == self.dragon.loc_y

  def check_treasure(self):
    return self.player.loc_x == self.treasure.loc_x and self.player.loc_y == self.treasure.loc_y

if __name__ == "__main__":
  g = Game()
  g.run()

** I know my L-R spacing lining up '=' is in contradiction to PEP-8, but I can't read code without doing it (mild dyslexia). But doing this has also proven beneficial and allowed for easily spotting errors in dict lookups and seeing typos/copy paste errors.

share|improve this answer
    
While I agree with all this answer has to say, it's also significantly more complex than the original code. A newbie is going to have a lot to comprehend if they're presented with this as the teacher's solution. – Carcigenicate Feb 28 at 16:51
    
So @Carcigenicate, I have mentored 5th graders in programming, and they do very good in understanding objects. You can say an object is an apple, it has a weight, color, size, etc. I was actually amazed at how well they grasped that over other things. And when it comes to logic, breaking stuff out into single responsibility functions actually helps them see how to build a program. I asked my student, what are the rules for ttt, what are the victory conditions, etc. and while they are producing more code, you can lead the student to generate that code just by asking pointed questions. – pyInTheSky Feb 29 at 15:12
    
each session we broke out a core concept/feature and it became a function, whether it was figuring out if a move was victorious, asking for player names, choosing who goes first, etc. We then docstringed each function and he could go back and say this 5 line block does this and so on ... vs a mash of code. – pyInTheSky Feb 29 at 15:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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