6
\$\begingroup\$

Tell me how I can make it more efficient, shorter. How I can make it adhere more to convention? Is there some stuff I'm doing that you would do in a better way?

The main gist is that I create and populate a 2d-array with objects, and use PyGame to display it graphically. We have a Hero, who is a Character object, that can move around the grid. The hero, internally, keeps track of his coordinates.

The update function scans the grid and checks for disagreements between the hero's internal coordinates and his place on the map (as a result of the move function). If there is a disagreement, it destroys the hero and resurrects him in the new location that agrees with his internal coordinates.

import random as random    
import pygame as pygame

pygame.init()                                 #start up dat pygame
clock = pygame.time.Clock()                   #for framerate or something? still not very sure
Screen = pygame.display.set_mode([650, 650])  #making the window
Done = False                                  #variable to keep track if window is open
MapSize = 25                                  #how many tiles in either direction of grid

TileWidth = 20                                #pixel sizes for grid squares
TileHeight = 20
TileMargin = 4

BLACK = (0, 0, 0)                             #some color definitions
WHITE = (255, 255, 255)
GREEN = (0, 255, 0)
RED = (255, 0, 0)
BLUE = (0, 0, 255)



class MapTile(object):                       #The main class for stationary things that inhabit the grid ... grass, trees, rocks and stuff.
    def __init__(self, Name, Column, Row):
        self.Name = Name
        self.Column = Column
        self.Row = Row


class Character(object):                    #Characters can move around and do cool stuff
    def __init__(self, Name, HP, Column, Row):
        self.Name = Name
        self.HP = HP
        self.Column = Column
        self.Row = Row

    def Move(self, Direction):              #This function is how a character moves around in a certain direction

        if Direction == "UP":
            if self.Row > 0:                #If within boundaries of grid
                if self.CollisionCheck("UP") == False:       #And nothing in the way
                   self.Row -= 1            #Go ahead and move

        elif Direction == "LEFT":
            if self.Column > 0:
                if self.CollisionCheck("LEFT") == False:
                    self.Column -= 1

        elif Direction == "RIGHT":
            if self.Column < MapSize-1:
                if self.CollisionCheck("RIGHT") == False:
                         self.Column += 1

        elif Direction == "DOWN":
            if self.Row < MapSize-1:
                if self.CollisionCheck("DOWN") == False:
                    self.Row += 1

        Map.update()       

    def CollisionCheck(self, Direction):       #Checks if anything is on top of the grass in the direction that the character wants to move. Used in the move function
        if Direction == "UP":
            if len(Map.Grid[self.Column][(self.Row)-1]) > 1:
                return True
        elif Direction == "LEFT":
            if len(Map.Grid[self.Column-1][(self.Row)]) > 1:
                return True
        elif Direction == "RIGHT":
            if len(Map.Grid[self.Column+1][(self.Row)]) > 1:
                return True
        elif Direction == "DOWN":
            if len(Map.Grid[self.Column][(self.Row)+1]) > 1:
                return True
        return False

    def Location(self):
        print("Coordinates: " + str(self.Column) + ", " + str(self.Row))


class Map(object):              #The main class; where the action happens
    global MapSize

    Grid = []

    for Row in range(MapSize):     # Creating grid
        Grid.append([])
        for Column in range(MapSize):
            Grid[Row].append([])

    for Row in range(MapSize):     #Filling grid with grass
        for Column in range(MapSize):
            TempTile = MapTile("Grass", Column, Row)
            Grid[Column][Row].append(TempTile)

    for Row in range(MapSize):     #Putting some rocks near the top
        for Column in range(MapSize):
            TempTile = MapTile("Rock", Column, Row)
            if Row == 1:
                Grid[Column][Row].append(TempTile)

    for i in range(10):          #Placing Random trees
        RandomRow = random.randint(0, MapSize - 1)
        RandomColumn = random.randint(0, MapSize - 1)
        TempTile = MapTile("Tree", RandomColumn, RandomRow)
        Grid[RandomColumn][RandomRow].append(TempTile)

    RandomRow = random.randint(0, MapSize - 1)      #Dropping the hero in
    RandomColumn = random.randint(0, MapSize - 1)
    Hero = Character("Hero", 10, RandomColumn, RandomRow)

    def update(self):        #Very important function
                             #This function goes through the entire grid
                             #And checks to see if any object's internal coordinates
                             #Disagree with its current position in the grid
                             #If they do, it removes the objects and places it 
                             #on the grid according to its internal coordinates 

        for Column in range(MapSize):      
            for Row in range(MapSize):
                for i in range(len(Map.Grid[Column][Row])):
                    if Map.Grid[Column][Row][i].Column != Column:
                        Map.Grid[Column][Row].remove(Map.Grid[Column][Row][i])
                    elif Map.Grid[Column][Row][i].Name == "Hero":
                        Map.Grid[Column][Row].remove(Map.Grid[Column][Row][i])
        Map.Grid[int(Map.Hero.Column)][int(Map.Hero.Row)].append(Map.Hero)

Map = Map()

while not Done:     #Main pygame loop

    for event in pygame.event.get():         #catching events
        if event.type == pygame.QUIT:
            Done = True       

        elif event.type == pygame.MOUSEBUTTONDOWN:
            Pos = pygame.mouse.get_pos()
            Column = Pos[0] // (TileWidth + TileMargin)  #Translating the position of the mouse into rows and columns
            Row = Pos[1] // (TileHeight + TileMargin)
            print(str(Row) + ", " + str(Column))

            for i in range(len(Map.Grid[Column][Row])):
                print(str(Map.Grid[Column][Row][i].Name))  #print stuff that inhabits that square

        elif event.type == pygame.KEYDOWN:
            if event.key == pygame.K_LEFT:
                Map.Hero.Move("LEFT")
            if event.key == pygame.K_RIGHT:
                Map.Hero.Move("RIGHT")
            if event.key == pygame.K_UP:
                Map.Hero.Move("UP")
            if event.key == pygame.K_DOWN:
                Map.Hero.Move("DOWN")

    Screen.fill(BLACK)

    for Row in range(MapSize):           # Drawing grid
        for Column in range(MapSize):
            for i in range(0, len(Map.Grid[Column][Row])):
                Color = WHITE
                if len(Map.Grid[Column][Row]) == 2:
                    Color = RED
                if Map.Grid[Column][Row][i].Name == "Hero":
                    Color = GREEN


            pygame.draw.rect(Screen, Color, [(TileMargin + TileWidth) * Column + TileMargin,
                                             (TileMargin + TileHeight) * Row + TileMargin,
                                             TileWidth,
                                             TileHeight])

    clock.tick(60)      #Limit to 60 fps or something

    pygame.display.flip()     #Honestly not sure what this does, but it breaks if I remove it
    Map.update()

pygame.quit()
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! To help reviewers review your code more easily, it would be great if you could add a description of the code into your question. You did a great job on the title, by the way. \$\endgroup\$ – zondo Aug 19 '16 at 3:32
  • \$\begingroup\$ Okay, I added some comments directly into the code. Hope that makes it easier to understand \$\endgroup\$ – Kos Aug 19 '16 at 3:49
10
\$\begingroup\$

Styling

Python has its own official style guide (written by the author of Python) called PEP 8. It has some things to say about your code. By the way, you can check your code against PEP 8 by installing the PyPi package or by trying it online.

Naming

It's great to see that you have constants instead of using those numbers manually. I am also happy to see that your constant names are ALLCAPS. I do notice, however, that some of your constants do not have that. For example, TileWidth is in PascalCase. I can see why you might consider it different: colors always have the same rgb values, but the tile width could change; but the important thing is that it doesn't change within the program. It starts as 20 and is always 20.

Many of your variables are in PascalCase, but not quite all of them (Map.update(), for example). Above all, be consistent. Okay, not above all; PEP 8 does mention that A Foolish Consistency is the Hobgoblin of Little Minds. It also recommends snake_case for method names and instance variables. That means, pretty much everything except constants and classes. Constants, as I mentioned before, are ALLCAPS. Classes are actually supposed to be how you have them: PascalCase.

Importing

When you import random, you are importing a module called random.py that is somewhere in your Python path. What is done with that module? It is called random. If you want to give a different name, you can do import random as rand, for example. You don't need to do import random as random. Your imports are still not how PEP 8 recommends:

Imports should be grouped in the following order:

  1. standard library imports
  2. related third party imports
  3. local application/library specific imports

You should put a blank line between each group of imports.

(Emphasis mine.)

Comparisons to booleans

All that an if block needs for a condition is something that can be interpred as Truthy or Falsey. It doesn't necessarily need to be the actual boolean values True and False. In fact, a common way to see if a list is empty uses that:

if mylist:
    print("It is populated!")
else:
    print("Nothing in here.")

You see, the ==, >, <, etc. don't need to be in it. When something returns a boolean value, you don't need to see if it is equal to True. Just check if it is Truthy. That means, use if ...: instead of if ... == True:, and use if not ...: instead of if ... == False:.

Comments

I would recommend that you read the PEP 8 section on comments. Well, I recommend that you read the whole PEP, but read this one especially. The inline comments are helpful, but a little too much. For example, one of your comments is:

#for framerate or something? still not very sure

If you aren't sure, don't bother to write the comment. In fact, anyone reading your code should look up the PyGame documention if he doesn't understand it. Your script is not a tutorial on third-party modules. You should have comments for the logic of your code, not for the third-party syntax.

There are also some comments that aren't needed because the code itself is obvious. For example, we all know that black, white, green, red, and blue are colors. We also know what an assignment looks like, so we don't need a comment that says:

#some color definitions

Don't feel obligated to write a comment for everything. Your goal should be to write code that is clear enough that it doesn't need comments. Of course, that can be hard to attain. If a comment is required, by all means include it. Don't skimp on comments, but don't wear out your keyboard.

PEP 8 also mentions that a comment should have a space between the hash sign and the first character of the comment. After all, the hash sign is not part of the first word, so it should have its own space.

It also mentions that inline comments should be used sparingly. It can get a little out of hand if your comments make your line 130 lines long, and then you look at the code in something that has wordwrap, so it looks all messed up. Then again, a text editor that doesn't wrap could be annoying. You might not like needing to scroll horizontally back and forth to see the comment then the code. It is often better to put the comment above the line it is commenting on.

Whitespace

I actually took my own advice and installed the PEP 8 package. Running your code through it brought to my attention some whitespace oddities. It noted some whitespace that was added to the end of some lines, and it also mentioned "too many blank lines" in a couple places. The PEP 8 package, of course, got its information from PEP 8 itself. You can find PEP 8's opinion here. I don't see it anywhere mention using three lines. In a couple places, you use even four lines, but PEP 8 recommends methods of a class to be separated by one blank line, and class and function definitions by two lines.

One more thing that it caught: your Character.Move function has some extra indentation when Direction == "RIGHT". It doesn't change the program flow, but it is stylistically displeasing.

Miscellaneous

You want to make this program customizable, right? There is no greater customization than being able to write a script that uses yours. To do that, it's helpful to create functions for different tasks, and run them only if your file is being run as a script instead of being imported as a module. To do that check, use if __name__ == '__main__':.


It's excellent that you are using object-oriented programming. You don't, however, seem to take much advantage of it in Map. You do a bunch of code at the class level. I would recommend that you define an __init__ method and please use instance attributes instead of global variables.


I see a lot of "DOWN", "UP", etc. You should use constants for these. Otherwise, one little typo could cause some unexpected, sometimes-hard-to-debug behavior.


Your Character.Move method has a lot of checks in it, and some of them are repeated. I would recommend this:

def move(self, direction):
    if self.collision_check(direction):
        return False

    # UP should be a global constant
    if direction == UP and self.row > 0:
        self.row -= 1
    ...

I would expect a method with the name Location to return the location of the hero, but it prints it instead. It is a lot more versatile if you make it return it and let whatever calls it print it however it wants to. If you really want to print it inside this method, I would recommend a more appropriate name such as print_location.


Your pygame.draw.rect(...) lines are a little long. I would suggest wrapping them more like this:

pygame.draw.rect(screen, color, [
   (tile_margin + tile_width) * column + tile_margin,
   (tile_margin + tile_height) * row + tile_margin,
    tile_width, tile_height
])

My preference is for the closing brackets to be on their own lines. That way, they serve as a sort of break between this group and the below lines.

\$\endgroup\$
  • \$\begingroup\$ The linter pep8 is now called pycodestyle (and is more up-to-date) this happened as Guido van Rossum asked for it to be renamed. \$\endgroup\$ – Peilonrayz Aug 19 '16 at 14:17
  • 1
    \$\begingroup\$ zondo, thank you very, very much for taking the time to write such a detailed response. I have a couple of questions that I will put out there for you, or anyone else, to answer. Why should I avoid global variables? How would you recommend I make more use of the map object (what kind of stuff should be initialized?) ? \$\endgroup\$ – Kos Aug 19 '16 at 15:13
  • \$\begingroup\$ @Kos: There are many places that give excellent answers to why you shouldn't use global variables. Just Google "global variables are evil". It's actually such a common search term that the words "evil" and "bad" are suggested when you type the rest of the search term. A good explanation that I found is on this question. My mention of initializing the map object was because you are creating the map at the definition of the class. Your class becomes more easily reusable if you create the map when a map object is instantiated. \$\endgroup\$ – zondo Aug 19 '16 at 20:38
3
\$\begingroup\$

Just to add a bit to @Zondo's answer:

  • At first glance one cannot know it CollisionCheck(self, direction) will return true if the player can move in that direction or if it is blocked. I think CanMove(self, direction) would be easier to read (of course it will have to return the opposite of what it returns now).

  • About the flip(): if you're not using double buffering, it will just update the display. If you're using double buffering, it's slightly more complicated. To keep it simple, your drawing operations will be made on off-screen memory, keeping the display as it is. When the drawing operations are completed, you will flip(), that is swap, the two buffers, so that the updated one becomes visible at the correct time of screen refresh. This helps having smooth animations.

  • You can have a dictionary for the movement directions

You can use something like:

KeyLookup = {
    pygame.K_LEFT: "LEFT",
    pygame.K_RIGHT: "RIGHT",
    pygame.K_DOWN: "DOWN",
    pygame.K_UP: "UP"
}

so that your check can become:

elif event.type == pygame.KEYDOWN:
    Map.Hero.Move(KeyLookup[event.key])

Which means that your movement function can also become simpler:

def Move(self, Direction):              #This function is how a character moves around in a certain direction
    if self.CollisionCheck(Direction) == True:       #Nothing in the way
        return

    if Direction == "UP":
        if self.Row > 0:                #If within boundaries of grid
           self.Row -= 1            #Go ahead and move

    elif Direction == "LEFT":
        if self.Column > 0:
            self.Column -= 1

    elif Direction == "RIGHT":
        if self.Column < MapSize-1:
            self.Column += 1

    elif Direction == "DOWN":
        if self.Row < MapSize-1:
            self.Row += 1

    Map.update()

I would also move the check for boundaries inside of the collision check, but that may depend on how you want to handle the two cases.

  • This is specific to the implementation, but it would probably be nice if you could hold down the direction button and see the player move, without repeatedly pressing the button. You can simply set a variable IsMoving on the KEYDOWN event and reset it on the KEYUP event.
\$\endgroup\$
  • \$\begingroup\$ Thanks about clarifying the flip() function! The dictionary is definitely something that I will implement. I planned on moving the check for boundaries into the collision check today. In the case of holding a key down, I have no idea how to make it so that the hero simply doesn't shoot across the screen when the key is pressed. I have to find a way to create some sort of time interval between the use of the move function when it is held down. Thanks alot for your response! \$\endgroup\$ – Kos Aug 19 '16 at 15:17
1
\$\begingroup\$

This is not so much an answer rather than an interesting suggestion for the "reasoning of objects" moving within a "shared environment". The suggestion is AOP (Agent Orientated Programming). This has been widely used for modelling complex adaptive systems (SWARM).

Essentially you create an encapsulated program (Agent) with its own logic about how to manouver in an environment (Grid world), and then launch it within an environment that it will share with other Agents. The beauty of this is that each Agent will judge where it will move next based on the environment it is in, the reasoning you can apply can be advanced or simple and different agents can have different properties (heterogeneous agents) and communication between agents is easy (an agent can broadcast propoerties of itself to change behaviour of surrounding agents. Eg. if your hero character is holding a weapon, other charaters may want to go to grid spaces further away!)

You can also make agents have meaningful movement rather than them moving around aimlessly, without having to implement complex and computationally demanding functions (you can supply a grid space end point which may be a shop or somthing in your environment, and the agent will navigate the environment to get there whilst dealing with obsticals on the way!).

I am not so familiar with python these days. more of a java person, for which there are really good AOP frameworks and Agent Development Environments (JADE, SARL, etc). but there is Python based frameworks too.

To give you an understanding of Agent based modelling, have a look at NetLogo Examples. NetLogo is an educational simulation software for agents with embedded rules.

For something directed towards implementation checkout RepastPy Agents I think there is a Grid agent already made with it ;)

\$\endgroup\$
  • \$\begingroup\$ Very interesting! It seems that this type of approach has been studied and refined far. I'll look into AOP, thanks for the enlightenment \$\endgroup\$ – Kos Aug 19 '16 at 15:18

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.