6
\$\begingroup\$

I wrote a simple game using python, but I wonder: how I can improve it? Any opinions will be useful.

I thought about optimization, since the program consumes 20% processor resources, but don't know where to begin.

Code of game:

import random
import time
import os

class LifeGame:
    def __init__(self):
        self.width = 20
        self.height = 20
        self.matrix = []

        row_counter = 0
        for y in range(self.height):
            self.matrix.append([])
            for x in range(self.width):
                self.matrix[row_counter].append(" ")
            row_counter += 1

    def print(self, iteration):
        life_cells = 0
        empty_cells = 0

        print("", "---"*self.width)
        for y in range(self.height):
            print("|", end='')
            for x in range(self.width):
                print(self.matrix[y][x], end='  ')
            print("|")
        print("", "---" * self.width)

        for y in range(self.height):
            for x in range(self.width):
                if self.matrix[y][x] == " ":
                    empty_cells += 1
                elif self.matrix[y][x] == "*":
                    life_cells += 1
        if empty_cells != 0:
            print(round((life_cells/100), 2)*(100/((self.width*self.height)/100)), "% of life")
        else:
            print("100 % of life")
        print(iteration)

    def spawn_life(self):
        rand_coords = []
        counter = 0
        for i in range(1):
            rand_coords.append([])
            rand_coords[counter].append(random.randint(0, self.width-1))
            rand_coords[counter].append(random.randint(0, self.height-1))
            counter += 1

        for i in range(len(rand_coords)):
            self.matrix[rand_coords[i][1]][rand_coords[i][0]] = "*"
        return rand_coords

    def get_available_coords(self, x, y):

        available_coords = [[x-1, y], [x+1, y], [x, y+1], [x, y-1]]
        coords = []
        for i in range(len(available_coords)):
            if available_coords[i][0] <= self.width-1 and available_coords[i][0] >= 0\
                and available_coords[i][1] <= self.height-1 and available_coords[i][1] >= 0\
            and self.matrix[available_coords[i][1]][available_coords[i][0]] == " ":
                coords.append([available_coords[i][0], available_coords[i][1]])

        return coords

    def add_life_to_matrix(self, coords):
        coords = sum(coords, [])
        for i in range(len(coords)):
            coord = coords[i]
            x, y = coord
            chance = random.randint(0, 100)
            if chance == 1:
                self.matrix[y][x] = '*'


lg = LifeGame()

lg.spawn_life()
iteration = 0
while True:
    coords = []

    for y in range(lg.width):
        for x in range(lg.height):
            if lg.matrix[y][x] == "*":
                coords.append(lg.get_available_coords(x, y))

    lg.add_life_to_matrix(coords)
    iteration += 1

    lg.print(iteration)
    time.sleep(0.01)
    os.system('cls')
\$\endgroup\$
2
  • \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. You can ask a follow up question with a link back to this question if you want your new code to be reviewed as well. \$\endgroup\$
    – pacmaninbw
    Commented Mar 30, 2023 at 16:20
  • \$\begingroup\$ Thank you for your advice! \$\endgroup\$ Commented Mar 30, 2023 at 16:23

2 Answers 2

9
\$\begingroup\$

Using attributes

Why is iteration passed into the LifeGame.print function? This makes more sense to me as an attribute on LifeGame which is incremented when you take steps. This means that each instance of LifeGame tracks its progress meaning I could in principle iterate multiple separately.

Initialising matrix

You initialise your 2D array with explicit loops.

       self.matrix = []

        row_counter = 0
        for y in range(self.height):
            self.matrix.append([])
            for x in range(self.width):
                self.matrix[row_counter].append(" ")
            row_counter += 1

If it were a single list, it would be much easier, faster and more clear to initialise it as

self.matrix = [" "]*self.height*self.width

N.B. This works because strings are immutable. This will create copies for mutable objects (such as our list, thanks to Alex F for pointing this out).

For mutable objects/generative functions, it would be better to use:

self.matrix = [[obj() for _ in range(self.width)] for _ in range(self.height)]

in this case, obj() should be our string " "

Looping

In Python, it is usual to iterate directly over the object (e.g. list)

for i in range(len(rand_coords)):
    self.matrix[rand_coords[i][1]][rand_coords[i][0]] = "*"

Could be:

for elem in rand_coords:
    self.matrix[elem[1]][elem[0]] = "*"

or even (using unpacking)

for x, y in rand_coords:
    self.matrix[y][x] = "*"

Which looks clearer to you?

You also have the strange statement

for i in range(1):

otherwise known as not looping. Is this deliberate?

Likewise:

    def add_life_to_matrix(self, coords):
        coords = sum(coords, [])
        for i in range(len(coords)):
            coord = coords[i]
            x, y = coord
            chance = random.randint(0, 100)
            if chance == 1:
                self.matrix[y][x] = '*'

simply becomes

    def add_life_to_matrix(self, coords):
        coords = sum(coords, []) # Is this an attempt to flatten this array? Not clear, could do with a comment       
        for x, y in coords:
            chance = random.randint(0, 100)
            if chance == 1:
                self.matrix[y][x] = '*'

We can also use list comprehensions to build arrays where needed

    def spawn_life(self):
        rand_coords = []
        counter = 0
        for i in range(1):
            rand_coords.append([])
            rand_coords[counter].append(random.randint(0, self.width-1))
            rand_coords[counter].append(random.randint(0, self.height-1))
            counter += 1

could be as simple as:

rand_coords = [[random.randint(0, self.width-1), random.randint(0, self.height-1)]
               for i in range(N)]

Counting

Python includes a lot of useful features in its stdlib, including collections.Counter which as its name suggests, counts things for you. Instead of:

        for y in range(self.height):
            for x in range(self.width):
                if self.matrix[y][x] == " ":
                    empty_cells += 1
                elif self.matrix[y][x] == "*":
                    life_cells += 1

we could do

# At the top of the file
from collections import Counter

cnt = Counter()
for row in self.matrix:
    cnt.update(row)
life_cells = cnt['*']
empty_cells = cnt[' ']

or even as a one-liner

cnt = Counter(pix for row in self.matrix for pix in row)

Using format magic

You have the line:

            print(round((life_cells/100), 2)*(100/((self.width*self.height)/100)), "% of life")

Did you know that Python includes % as a format type?

print(f"{life_cells/(self.width*self.height):%}% of life")

Much clearer (You can use .0% if you want it rounded to 0 places).

We can also use magic to print the array, (though as J_H rightly points out, this would be better done with some kind of ncurses or pygame libraries)

        print("", "---"*self.width)
        for y in range(self.height):
            print("|", end='')
            for x in range(self.width):
                print(self.matrix[y][x], end='  ')
            print("|")
        print("", "---" * self.width)

becomes

# Somewhere constant
BORDER = "---"*self.width
MAIN = "|"+" {} "*self.width+"|"

print(BORDER)
for row in self.matrix:
    print(MAIN.format(*row))
print(BORDER)

alternatively you could do:

print("---"*self.width)
for row in self.matrix:
    print(f"| {' '.join(row)} |")
print("---"*self.width)

Which may be faster as format is slow

\$\endgroup\$
4
  • \$\begingroup\$ You mention how one should use [[obj() for _ in range(self.width)] for _ in range(self.height)] for mutable objects, yet you suggest to create several copies of the same list with [[" "] * self.width] * self.height... \$\endgroup\$
    – Alex F
    Commented Mar 31, 2023 at 13:52
  • \$\begingroup\$ Good point! I forgot that. Will fix it. \$\endgroup\$ Commented Mar 31, 2023 at 13:56
  • \$\begingroup\$ @DeathIncarnate [" "]*self.height*self.width is a list of strings, not a list of lists; i.e. the matrix is flattened to 1D, not 2D \$\endgroup\$
    – obataku
    Commented Apr 1, 2023 at 2:34
  • \$\begingroup\$ @obataku Hence why I said "If it were a single list," \$\endgroup\$ Commented Apr 1, 2023 at 12:08
6
\$\begingroup\$

You explain that you wish to improve CPU consumption but you

don't know where to begin.

Always start optimizing by asking "where did the cycles go?" Run the profiler:

$ python -m cProfile -s tottime life.py

Also, a numba @jit on add_life_to_matrix wouldn't hurt.

You are using list-of-lists, which is very pythonic, but it involves lots of random reads for object pointer chasing. Consider using a cache-friendly array instead, or perhaps an ndarray.

As with any project, definitely record timing measurements before and after a refactor, so you're sure it had the desired effect.


lg = LifeGame()
lg.spawn_life()

Looks great!

iteration = 0
while True:

Consider pushing iteration down into the game object.

You compute a very nice coords data structure. But it feels like that computation should happen within the game class, perhaps as a local variable of an .update() method.

    lg.print(iteration)
    time.sleep(0.010)
    os.system('cls')

I imagine there's a lot of flicker associated with that.

Rather than a heavyweight clearscreen approach, you could just send an erase command: <esc>[2J

Better, you could position the cursor at top-of-screen and rewrite cells over displayed cells, to reduce flicker.

Or consider using pygame pixels to represent live cells.


In spawn_life you jump through a bunch of hoops to create a temporary data structure and then consume it. It's unclear why we'd want to create it in the first place. Just pick a random (x, y) and place a new * star there.

        for i in range(1):

That's an odd way to say "do the following exactly once."

I assume it's leftover from debug, and that 1 is a magic number that you sometimes change. Bury that constant in the signature instead:

    def spawn_life(self, num_births=20):
        ...
        for _ in range(num_births):
            x, y = randint...
            self.matrix[y][x] = "*"

No need for a return value that will just be ignored.


I tend to think of get_available_coords as "get neighbor coords", but whatever, that works fine.

        available_coords = [[x-1, y], [x+1, y], [x, y+1], [x, y-1]]

Rather than list of lists, the pythonic way to phrase that would be list of 2-tuples:

        available_coords = [(x-1, y), (x+1, y), (x, y+1), (x, y-1)]

We use list for things that are all "the same", and tuple for things with fixed number of elements where element position affects the meaning. Think of it as an immutable C struct.

Wow, that if is quite the mouthful. First let's use black to make it legible:

            if (
                available_coords[i][0] <= self.width - 1
                and available_coords[i][0] >= 0
                and available_coords[i][1] <= self.height - 1
                and available_coords[i][1] >= 0
                and self.matrix[available_coords[i][1]][available_coords[i][0]] == " "
            ):
                coords.append([available_coords[i][0], available_coords[i][1]])

That available_coords[i] expression is tediously long and we use it several times, so let's assign temp vars:

            x, y = available_coords[i]

Notice that element [0] went into x and [1] into y.

Now let's do some per-axis conjunct chaining:

            if (
                0 <= x < self.width
                and 0 <= y < self.height
                and self.matrix[y][x] == " "
            ):

In add_life_to_matrix the combination of these two

            chance = random.randint(0, 100)
            if chance == 1:

amounts to a Magic Number of .01. Better to put it in the method signature, so unit tests or other callers can modify it when desired.


You asked about making this go faster, presumably for a larger universe than 20 x 20 cells. In 1970 Martin Gardner wrote about the game Conway devised, and a rich literature has sprung up around it since. It includes sightings of Gliders and of larger beasts, descriptions of how to find and construct such beasts, and algorithms to efficiently compute the next generation or the K-th subsequent generation.

Consider a canvas of more than 1000 x 1000 cells, with a glider gun near the origin that keeps pumping out new gliders. Out of a million cells, most will be dead in this generation, and in every subsequent generation, so they need not be recomputed. Even if the original setup arranged for some isolated beehives to be present, they won't change either. Segmenting the universe into interesting and unchanging sections is key to efficiently computing large Life scenarios. Typically we only refresh the display every K iterations, so even an isolated blinker or other cyclic beast may be ignored for a while, until we are at last asked to render its neighborhood in the K-th generation.


This code achieves many of its design goals. Some of the responsibilities currently shouldered by callers could sensibly be pushed down into the game class.

I would be happy to delegate or accept maintenance tasks for this codebase. There are some recommended code cleanups that really ought to happen before merging to main.

\$\endgroup\$
1
  • \$\begingroup\$ Numba looks very interesting, thanks for mentioning it. It'll probably become a standard part of my toolkit in the future. \$\endgroup\$
    – Kaia
    Commented Mar 30, 2023 at 23:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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