[Tutor] Improving My Simple Game Code for Speed, Memory and Learning

[Steven D'Aprano]

On Fri, Jan 02, 2015 at 09:00:22PM -0500, WolfRage wrote:
> Python3.4+ Linux Mint 17.1 but the code will be cross platform (Mobile, 
> Windows, Linux, OSX).
> 
> First an explanation of how the game works: The game is a simple 
> matching game but with a twist. Instead of matching a straight 3 in a 
> row, we have some rules that only certain combinations will result in an 
> elimination. In general 2 of the same value in a row with with 1 of 2 
> other possible values will eliminate all three nodes. 

Do you mean "1 of 2" or "1 or 2"?


> Eliminations can 
> occur either horizontally or vertically but not diagonally. Each tile 
> has a value now, that value is used when evaluating the rules. Currently 
> there are only 5 allowed values of 0-4 although more could be added 
> later, so any solution needs to be expandable.

Okay, so the values are 0, 1, 2, 3, and 4. This doesn't match your code 
belong. Is the code wrong or your description?


> These are the basic rules that allow for an elimination to occur(values 
> are put in quotes to distinguish values):
> 2 "5"s followed or proceeded by a "19" will eliminate all 3 nodes.
[...]

Since neither 5 nor 19 is a possible value, this can never apply.

All the other rules you give include values greater than 4, and likewise 
can never apply. That means that no eliminations are ever possible. I 
think your description needs some work :-)


> class GameTile():
[...]
>     def __str__(self):
>         #return '%d, %d' % (self.col, self.row)
>         if len(str(self.value)) == 1:
>             return ' ' + str(self.value)
>         return str(self.value)

Eliminate comments which no longer apply. Don't use them for keeping 
old, obsolete or wrong code.

This method can be better written like this:

    def __str__(self):
        return "%2d" % self.value


> class GameGrid():
>     def __init__(self, cols=8, rows=7, **kwargs):
>         self.cols = cols
>         self.rows = rows
>         self.make_grid()
> 
>     def make_grid(self):
>         # grid is 2d array as x, y ie [x][y].
>         self.grid = []
>         for row_num in range(self.rows):
>             self.grid.append([GameTile(value=random.choice([5, 6, 11, 
> 19, 20]), col=col_num,
>                 row=row_num) for col_num in range(self.cols)])

That's rather gnarly code. If you're going to do that, it's probably 
nice to use a temporary variable to clarify your intention:

        for row_num in range(self.rows):
            row = [GameTile(
                   value=random.choice([5, 6, 11,19, 20]), col=col_num, 
                   row=row_num) for col_num in range(self.cols)
                   ]
            self.grid.append(row)


Which is still pretty gnarly. You only have three arguments to GameTile, 
why do you need to use keyword arguments? This is neater and less 
noisy, although it does require you get the order right:

        # untested
        for row_num in range(self.rows):
            row = [GameTile(row_num, col_num, random.choice([5, 6, 11,19, 20]) 
                   for col_num in range(self.cols)
                   ]
            self.grid.append(row)

Even better:

        for row_num in range(self.rows):
            self.make_row(row_num)

    def make_row(self, row_num):
        values = self.allowed_values   # [5, 6, 11,19, 20]
        row = [GameTile(row_num, col_num, random.choice(values) 
               for col_num in range(self.cols)]
        self.grid.append(row)


Notice that the values are stored in an attribute, for ease of 
maintanence in the future when you extend them.

 
>     def find_eliminations(self):
>         # I define 3 variables for holding the 3 nodes/tiles that I am
>         # currently checking to see if an elimination possibility exists.
>         # It uses a math trick to check for elimination by adding the values
>         # and checking for specific totals. None of the possible totals overlap.

Sorry, I'm too tired to make sense of this function right now :-(

 
>     def check_total_and_eliminate(self, first, second, third):
>         total = None
>         if first.value == second.value:
>             total = first.value + second.value + third.value
>         elif second.value == third.value:
>             total = first.value + second.value + third.value
>         if total == 17 or total == 21 or total == 28 or total == 29 or \
>             total == 31 or total == 42 or total == 45 or total == 46 \
>             or total == 49 or total == 58:
>             first.eliminated = True
>             second.eliminated = True
>             third.eliminated = True

I think this method does too much. I would prefer to see it split into 
two methods, one to check the total, and the other to eliminate the 
cells if needed:

    def eliminate(self, first, second, third):
        first.eliminated = True
        second.eliminated = True
        third.eliminated = True

    def check_total(self, first, second, third):
        if first.value == second.value or second.value == third.value:
            total = first.value + second.value + third.value
        return total in (17, 21, 28, 29, 31, 42, 45, 46, 49, 58)


which you then use:

     if self.check_total(first, second, third):
         self.eliminate(first, second, third)




-- 
Steven