6
\$\begingroup\$

One of my first serious Python programs, realized Conway's Game of Life in Python. Not sure that get_cell_state and update_table functions are good as they might be. Would be glad to see any improvements or critique. Thank you for reviewing ^^.

import time
import random
import copy
import os


black_square_symbol = chr(int('25A0', 16))  # a nice unicode black square symbol


def get_cell_state(table, row, col):
    if row in range(len(table)) and col in range(len(table[0])):
        # little hack to return 0 if row and col are out of rang
        return 1 if table[row][col] else 0
    return 0


def get_neighboring_cells(table, row, col):
    sum = 0
    for row_shift in (-1, 0, 1):
        for col_shift in (-1, 0, 1):
            if row_shift or col_shift:
                #  checking for funcion to not check the state of the cell itself
                sum += get_cell_state(table, row + row_shift, col + col_shift)
    return sum


def generate_table(height, width, rand_seed=time.time()):
    random.seed(rand_seed)  #  giving a seed if user specifies
    table = [[None] * width for _ in range(height)]  # generating the table frame
    for row in range(height):
        for col in range(width):
            table[row][col] = random.choice([True, False])
    return table


def update_table(table, height, width):
    new_table = copy.deepcopy(table)  # deep copy to avoid mutability issues
    for row in range(height):
        for col in range(width):
            neighboring_cells = get_neighboring_cells(table, row, col)
            if neighboring_cells < 2 and table[row][col]:
                # underpopulation
                new_table[row][col] = False
            elif neighboring_cells > 3 and table[row][col]:
                # overpopulation
                new_table[row][col] = False
            elif neighboring_cells == 3 and not table[row][col]:
                # replication
                new_table[row][col] = True
    return new_table


def print_table(table):
    os.system('cls')  # clearing up the screen to print new table
    for row in table:
        for elem in row:
                print(black_square_symbol if elem else ' ', end='')
        print()  # newline


def main():
    os.system('color f0')  # making the background white and text black
    height = int(input('Enter table width: '))
    width = int(input('Enter table height: '))
    rand_seed = input('Enter seed to generate the table(leave blank if dont want to specify')
    if rand_seed:
        table = generate_table(width, height, float(rand_seed))
    else:
        table = generate_table(width, height)
    year = 0
    while True:
        print_table(table)
        print('year:', year)
        year += 1
        table = update_table(table, height, width)
        time.sleep(1)


if __name__ == '__main__':
    main()
\$\endgroup\$
  • 1
    \$\begingroup\$ Each iteration of this program makes a new table and checks every square on it, that's expensive! Most of the cells are not going to change state every iteration. One quick optimization you can do is to keep track of a list of all of the cells that changed in the previous iteration, then you only need to check those cells and their neighbors. \$\endgroup\$ – GDanger Apr 20 '18 at 20:35
  • 1
    \$\begingroup\$ As I always recommend when this comes up: if you are a Life enthusiast you should look into Gosper's Algorithm. You'll learn about functional programming, memoization techniques, and discover that you have the amazing ability to compute life boards with more cells than you have bits of memory, and move boards forwards generations faster the bigger the boards get. \$\endgroup\$ – Eric Lippert Apr 20 '18 at 21:50
  • 1
    \$\begingroup\$ @EricLippert en.m.wikipedia.org/wiki/Hashlife \$\endgroup\$ – redfast00 Apr 20 '18 at 21:54
  • \$\begingroup\$ @redfast00: That's the one. The hashlife implementation is great but it's super fun to implement the algorithm yourself. \$\endgroup\$ – Eric Lippert Apr 20 '18 at 21:57
  • \$\begingroup\$ I've don't remember where exactly I saw it off-hand, but there's an implementation that's like 5 lines. \$\endgroup\$ – jpmc26 Apr 20 '18 at 22:06
7
\$\begingroup\$

Quite nicely structured. some remarks

  • main() - while you have the guard and also an extra printing function there is a little mess in main(). It has got some game logic (the loop), it has got some UI (input and printing). split it into a run() function with parameters and a interactive main(). parameters to run() should be an initial board and the visualisation callback.

  • some non-portable OS calls - while acceptable for a test program you should think of a better solution. functions used by an importer should not depent on that. there are also patterns to throw import errors on wrong OS to give correct and immediate information.

  • seeding - usually shall be done once at program start only. you gave the user the chance to reproduce a board - nicely done. but what about the fallback (which btw. gives a ValueError currently)? What happens if someone imports your module and tries to generate random boards without seed? you will generate identical boards. check why, this is a very important lesson.

  • deepcopy - you don't need a copy, you want an empty table. this is confusing. implement something like empty_table()

\$\endgroup\$
7
\$\begingroup\$

There is one bug:

    table = update_table(table, height, width)

in the “main loop” should be

    table = update_table(table, width, height)

otherwise the program will crash for non-square tables.

If you compute the new cell state in a separate function

def new_state(table, row, col):
    num_neighbors = get_neighboring_cells(table, row, col)
    if table[row][col]:
        return num_neighbors == 2 or num_neighbors == 3
    else:
        return num_neighbors == 3

then computing the next-generation table can be done with (nested) list comprehension

def update_table(table, height, width):
    return [[new_state(table, row, col) for col in range(width)] for row in range(height)]

instead of nested for loops and an if/elif/elif chain. This makes also the deep copy (and thus the import copy) unnecessary.

The same list comprehension can be used when generating the initial table:

def generate_table(height, width, rand_seed=time.time()):
    random.seed(rand_seed)  #  giving a seed if user specifies
    return [[random.choice([True, False]) for _ in range(width)] for _ in range(height)]

The table width/height is not treated consistently:

update_table(table, height, width)

takes it as parameters, but

get_cell_state(table, row, col)

determines it from the table itself. Always computing width and height from the table would be one way to remove this inconsistency.

Another way would make the table “know” its width and height. In other words: make it a class with attributes and consequently, make the global functions instance methods of that class.

The Python way of printing an object is to implement the object.__str__(self) method, and then simply call print(table).

This is what it would look like (without changing your logic otherwise):

class Table:

    def __init__(self, width, height, rand_seed=time.time()):
        self.width = width
        self.height = height
        self.cells = [[random.choice([True, False]) for _ in range(width)] for _ in range(height)]

    def cell_state(self, row, col):
        if row in range(self.height) and col in range(self.width):
            return 1 if self.cells[row][col] else 0
        else:
            return 0

    def next_state(self, row, col):
        sum = 0
        for row_shift in (-1, 0, 1):
            for col_shift in (-1, 0, 1):
                if row_shift or col_shift:
                    sum += self.cell_state(row + row_shift, col + col_shift)
        if self.cells[row][col]:
            return sum == 2 or sum == 3
        else:
            return sum == 3

    def update(self):
        new_cells = [[self.next_state(row, col) for col in range(self.width)] for row in range(self.height)]
        self.cells = new_cells

    def __str__(self):
        return "\n".join(["".join([black_square_symbol if elem else ' ' for elem in row]) for row in self.cells])

and a (simplified version of the) main loop would then be

table = Table(width, height)
while True:
    os.system('cls')  # clearing up the screen to print new table
    print(table)
    table.update()
    time.sleep(1)
\$\endgroup\$
3
\$\begingroup\$

As Stefan said nicely structured, some additional remarks

get_cell_state can be made more readable (without mention hack):

def get_cell_state(table, row, col)
   height = len(table)
   width = len(table[0])
   if row >= 0 and row < height and col >= o and col < width:
        return 1 if table[row][col] else 0
    return 0

The same assignments for height and witdh can be used in the function update_table so that they can be skipped as parameters.

\$\endgroup\$

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.