8
\$\begingroup\$

This is my implementation of Conway's Game of Life in Python. Now since I am a novice coder, naturally I have some key doubts:

  1. The usage of idioms and code redundancies - Are there any small fragments of the program which can be better written?
  2. The usage of sys.argv - Is my usage of system arguments acceptable? Eg: $ python 02 1000 would mean taking input from 'input02.txt' and running until 1000 generations.
  3. Finally, in the get_input_matrix method, I use several reversals and appends in order to add empty rows and columns all around the input matrix. What could be better ways to approach this?

Along with all these, I am also wondering if variables are aptly named and the code is well-formatted/well-written, etc.

#!/usr/bin/python

from time import sleep
import sys


# This method is used to input the contents of the input file.
# If a matrix:
# 0 0
# 0 1
# is present in the input file,
# this generates: (adds 'buffer region' on all sides of the input)
# 0 0 0 0
# 0 0 0 0
# 0 0 1 0
# 0 0 0 0
# and returns it to the program.
def get_input_matrix(f='input.txt'):
    # Attempt opening user's input file.
    # If gives error, use default input file.
    try:
        inputFile = open(f, 'r')
    except:
        print r"Error: Invalid filename. Using 'input.txt'"
        inputFile = open('input.txt', 'r')
    finally:
        matrix = inputFile.readlines()
        temp = []
        for line in matrix:
            temp_line = [0]
            for element in line[:-1].split(' '):
                temp_line.append(int(element))
            temp_line.append(0)
            temp.append(temp_line)
        buffer_array = [0 for i in range(len(temp_line))]
        temp.append(buffer_array)
        temp.reverse()
        temp.append(buffer_array)
        temp.reverse()
        return temp


# Prints matrix
# 0 1
# 1 0
# as
#   *
# *
def print_matrix(matrix, height, width):
    for i in range(1, height-1):
        for j in range(1, width-1):
            if matrix[i][j] == 1:
                print '*',
            else:
                print ' ',
        print ""


def matrix_copy(matrix, height, width):
    mnew = [[matrix[i][j] for j in range(width)] for i in range(height)]
    return mnew


# Creates the next generation of the matrix.
def to_next_generation(matrix, height, width):
    temp = matrix_copy(matrix, height, width)
    for i in range(1, height-1):
        for j in range(1, width-1):
            count = matrix[i-1][j] + matrix[i-1][j-1] + \
                matrix[i][j-1] + matrix[i+1][j-1] + matrix[i+1][j] + \
                matrix[i+1][j+1] + matrix[i][j+1] + matrix[i-1][j+1]
            if count == 2:
                temp[i][j] = matrix[i][j]
            elif count == 3:
                temp[i][j] = 1
            elif count < 2 or count > 3:
                temp[i][j] = 0
    matrix = matrix_copy(temp, height, width)
    return matrix


if __name__ == '__main__':

    # python main.py 02
    # would open input02.txt
    if len(sys.argv) > 1:
        inputFile = "input" + sys.argv[1] + ".txt"
    else:
        inputFile = raw_input("Input file: ")

    # main matrix for operations
    matrix = get_input_matrix(inputFile)
    height, width = (len(matrix), len(matrix[0]))
    # prints actual height and width of the computation region
    print "Height = %d\nWidth = %d" % (height - 2, width - 2)

    # python main.py xx 1000
    # would run the code 1000 times
    if len(sys.argv) > 2:
        genLimit = genLimitOriginal = int(sys.argv[2])
    else:
        genLimit = genLimitOriginal = int(raw_input("Enter number \
            of generations to be evaluated: "))

    # list stores upto 9 previous generations
    # thus, has a queue like action
    prev_generations = []

    Generation = 0
    while Generation <= genLimit:

        # pushing the current generation into the queue
        if len(prev_generations) < 10:
            prev_generations.append(matrix)
        else:
            prev_generations.remove(prev_generations[0])
            prev_generations.append(matrix)

        # printing the current generation
        print "## Generation %d ##" % Generation
        print_matrix(matrix, height, width)
        # moving to the next generation
        matrix = to_next_generation(matrix, height, width)

        # delay in computation
        sleep(0.05)

        # if the computation isn't halted before the computation limit
        # is reached, it may restart for the same amount of time if the
        # user wants.
        # Eg: if the number of generations to be evaluated was 1000
        # and the user decides to continue, 1000 more generations will
        # be evaluated by the program.
        if Generation == genLimit:
            ch = raw_input("Do you want to continue? (Y/N): ")
            if ch == 'y' or ch == 'Y':
                genLimit += genLimitOriginal
            else:
                break

        Generation += 1

        # if the current generation has occured in any of the past nine
        # generations, it will definitely repeat itself. Thus, if such a
        # case occus, the computation is halted.
        if matrix in prev_generations:
            Generation += 1
            print "## Generation %d ##" % Generation
            print_matrix(matrix, height, width)
            print "Halted at Generation %d" % Generation
            break

Kindly also let me know if any other changes can be implemented to make this a better program.

\$\endgroup\$
3
\$\begingroup\$

You have extensive commenting about the usage and intent of functions, but they're just comments. You should make docstrings. Docstrings are like comments, but programmatically accessible.

>>> def help_me():
    """Isn't this helpful?"""

>>> help(help_me)
Help on function help_me in module __main__:

help_me()
    Isn't this helpful?

I would also recommend trimming some of the comments to be more concise:

"""
This method is used to input the contents of the input file.

0 0
0 1

Adds 'buffer region' on all sides of the input and generates:

0 0 0 0
0 0 0 0
0 0 1 0
0 0 0 0
"""

Next, using a default input is a great idea for when errors occur, but you've made a couple of mistakes in how you do this. First of all, you're defining f='input.txt.' as a default filename. That's a good practice, except that you've hardcoded the name in multiple places including using it as a fallback value. Instead of what you're doing now, I'd have a constant

DEFAULT_INPUT = 'input.txt'

and now use that everywhere you need the string. Default f to an empty string which will just fail and trigger the same error as an invalid filename. Here's how that change looks:

def get_input_matrix(f=''):
    """Attempt opening user's input file, fall back on default file."""

    try:
        inputFile = open(f)
    except:
        print "Error: Invalid filename. Using '{}'".format(DEFAULT_INPUT)
        inputFile = open(DEFAULT_INPUT)

I also removed 'r' from open, since r is the default value anyway there's no need to state it explicitly. Though being explicit is usually good, in this case inputFile is only used for a single line. And it's unambiguously just to be read from. If it was hard to discern what the intent was, using an explicit 'r' would clarify, but it's easy to see the intent without it here.

Likewise, you don't need r before your string. It's only necessary for preventing escape characters affecting the resulting string, but since you have no backslashes this is not an issue here.

Before we move on though, you should not be using a blanket except. This will mean that any error whatsoever can be caught and ignored. What if the user is passing actual file objects instead of strings? Surely that's a foolish enough mistake that you want to highlight it. Where possible, you should except a specific error. In this case, except IOError. This means that IOErrors (raised by invalid file names) will open your default file, but all other errors will be raised as normal.

Next, your finally block. It's not really necessary. You may have misunderstood what finally is for, but it ensures that no matter what happens in your try and except blocks, the finally block always gets executed. It's usually used for things like closing up files and other resources that shouldn't be left open. However you're using it as if everything is running smoothly. You don't need this block at all, to be honest. Just leave it as regular code.

That said, I would move your line where you read the file, back up into the try and except. And instead of using open, use what's called a context manager.

    try:
        with open(f) as inputFile:
            matrix = inputFile.readlines()

    except IOError:
        print "Error: Invalid filename. Using '{}'".format(DEFAULT_INPUT)
        with open(DEFAULT_INPUT) as inputFile:
            matrix = inputFile.readlines()

This with syntax is similar to open except that it will always ensure that files are properly closed, no matter what happens. You don't actually remember to call inputFile.close(), so using with would at the very least solve that problem.

As for your matrix reading, it could be simplified a bit. Instead of using a loop to split and call int on each element, you could use map. map calls a function on each element of a collection, so instead of

for element in line[:-1].split(' '):
    temp_line.append(int(element))

you can do

temp_line += map(int, line[:-1].split(' '))

But now you can make the lines before and after this even simpler, by adding 0 at either end:

temp = []
for line in matrix:
    temp_line = [0] + map(int, line[:-1].split(' ')) + [0]
    temp.append(temp_line)

At this point, we have a prime candidate for a list comprehension. List comprehensions are like for loops collapsed into a single expression that build a list. You can basically create your full temp list in one go, just by rearranging the above lines to this:

temp = [[0] + map(int, line[:-1].split(' ')) + [0] for line in matrix]

That line essentially does the same as the above loop, but faster and neater.

Also you don't need to reverse the list to add a line to the start. Use list.insert(0, line) to add an element at index 0, ie. the start.

\$\endgroup\$
  • \$\begingroup\$ Agree with almost all of that (in fact deleted quite a lot of my draft answer since it was the same and you beat me to the post!). The only bit I'm not sure about is using with open(f) as inputFile: in the context manager. Why not keep the r on the principle that "explicit is better than implicit"? \$\endgroup\$ – Jamie Bull Feb 15 '16 at 15:48
  • 1
    \$\begingroup\$ @JamieBull If explicit was always better then there would be no need for 'r' to be default. In this case, inputFile is only used for a single line. And it's unambiguously just to read from. If it was hard to discern what the intent was, using an explicit 'r' would clarify, but it's easy to see the intent without it here. \$\endgroup\$ – SuperBiasedMan Feb 15 '16 at 16:00
  • \$\begingroup\$ Ok, just wondering about the justification. I rarely see open for reading in the wild without the second parameter so thought it worth mentioning. \$\endgroup\$ – Jamie Bull Feb 15 '16 at 16:03
  • 1
    \$\begingroup\$ @JamieBull It's a good question, so I updated the explanation in my answer. \$\endgroup\$ – SuperBiasedMan Feb 15 '16 at 16:54
  • 1
    \$\begingroup\$ @IshaanSaxena Glad to help! Yes, map is similar to your list comprehension example, new_list = [int(element) for element in list]. It returns a list where every element has had the function int applied to it. But it doesn't work specifically on type, it could use another function in place of int. It is faster than a list comprehension and debatably more readable. \$\endgroup\$ – SuperBiasedMan Feb 17 '16 at 14:35
3
\$\begingroup\$

In addition to @SuperBiasedMan's answer:

Names

This is just stylistic, but PEP8 says that variable and function/method names should be in lower_underscore case, not camelCase, input_file rather than inputFile.

Checking if variable is one of several options

This is really just giving you options, but where you have:

if ch == 'y' or ch == 'Y':
    # do something

You could have:

if ch in ['y', 'Y']:
    # do something

This is a little easier to read, and adding another option is easier (such as if ch in ['y', 'Y', 'yes', 'YES']:.

My preference would be to convert to lower case and check for yes as well as y:

if ch.lower() in ['y', 'yes']:  # convert to lower case before checking
    # do something

Main method

In my opinion you're doing to much after the if __name__ == "__main_" and before calling your functions. It is more common just to handle the args there, then use them to call a main method.

In your program that might be up to the point where you have input_file, then you would call main(input_file, num_generations).

Handling system arguments

There are a couple of good libraries for calling programs from the command line. The one I usually use is argparse.

import argparse 
....

if __name__ == "__main__":
    parser = argparse.ArgumentParser(description="Conway's Game of Life.")
    parser.add_argument('--inputfile', metavar='inputfile', type=str,
                    help='Input file containing a matrix')
    # do the same to add a num_generations argument

    args = parser.parse_args()
    if args.inputfile:
        input_file = args.input_file
    else:
        input_file = raw_input("Input file: ")
    main(input_file, args.num_generations)
\$\endgroup\$
  • \$\begingroup\$ So if I use the Argument Parser, would the input arguments looks something like --inputfile file.txt? \$\endgroup\$ – Ishaan Saxena Feb 17 '16 at 13:15
  • \$\begingroup\$ Exactly, that's right. You can also register positional arguments though. See the docs docs.python.org/2/library/argparse.html \$\endgroup\$ – Jamie Bull Feb 17 '16 at 13:36
1
\$\begingroup\$

return directly

mnew = [[matrix[i][j] for j in range(width)] for i in range(height)]
return mnew

Can be just:

return [[matrix[i][j] for j in range(width)] for i in range(height)]

Heavy matrix dimension passing

Passing the dimension around all the time feels like an unnecessary burden, Python lists know their length:

>>> x = [i for i in range(1, 10**8)]
>>> len(x) # Takes an instant
99999999

You can just pass the matrix as an argument to simplify code re-use, the performance impact will be tiny.

Be generous with writing functions

For example getting the neighbours of a point in a matrix or calculating the the next alive/dead state for the individual cell are self-contained blocks of functionality, deserving to be functions.

Factor out the loops

You loop over the matrix twice in the exact same way, just write a function for it:

def enumerate_2d(matrix):
    for y in enumerate(matrix):
        for x in enumerate(metrix[0]):
            yield x, y, matrix[x][y]

And use it, you will also reduce indentation, that is always nice.

\$\endgroup\$
1
\$\begingroup\$

I'd consider using NumPy arrays to handle your matrices instead of native Python lists. It's tailored to use cases like this, and your to_next_generation() function will run considerably faster for larger matrices. With NumPy, you don't need to loop over rows or columns yourself. NumPy built-in functions will take care of it for you. It will be doing similar loops, but in C, so it will be faster than in Python.

Your code block:

# Creates the next generation of the matrix.
def to_next_generation(matrix, height, width):
    temp = matrix_copy(matrix, height, width)
    for i in range(1, height-1):
        for j in range(1, width-1):
            count = matrix[i-1][j] + matrix[i-1][j-1] + \
                matrix[i][j-1] + matrix[i+1][j-1] + matrix[i+1][j] + \
                matrix[i+1][j+1] + matrix[i][j+1] + matrix[i-1][j+1]
            if count == 2:
                temp[i][j] = matrix[i][j]
            elif count == 3:
                temp[i][j] = 1
            elif count < 2 or count > 3:
                temp[i][j] = 0
    matrix = matrix_copy(temp, height, width)
    return matrix

could become something like this in NumPy (code borrowed and modified from this Code Review answer.)

import numpy as np
from scipy.signal import convolve2d

def iterate_game_of_life(matrix, neighbors_kernel=None):
    """Performs one iteration of Conway's game of life on a 2d numpy array of bools"""
    if neighbors_kernel is None:
        neighbors_kernel = np.ones(shape=(3, 3), dtype='int')
        neighbors_kernel[1, 1] = 0

    neighbors_count = convolve2d(matrix, neighbors_kernel, mode='same')
    has_three, has_two = neighbors_count == 3, neighbors_count == 2
    return np.logical_or(has_three, np.logical_and(matrix, has_two))

In NumPy, you can tell the interpreter that your game board will always consist of only 1s and 0s by setting the dtype to be bool. In contrast, native Python always has to be on guard for an addition to the list of arbitrary type, and so will take much larger amounts of memory than Python lists.

\$\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.