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

[WolfRage]

On 01/03/2015 10:09 AM, Steven D'Aprano wrote:
> 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"?
1 of 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?
>
My description was wrong, I am sorry I had started to simplify my code 
when I was first writing this, but realized that ruined my algorithm for 
finding matches.That is probably where I confused Dave.
>
>> 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.

Sorry again, these are actually the correct values. But my earlier 
mistake misleads on this section that is actually correct.
>
> 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 :-)
>
Yes. Sorry. Let me re-do that.

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. 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 [5, 6, 11, 19, 20] although more 
could be added [52, 61, 67, etc] later, so any solution needs to be 
expandable.
The remainder of the explanation was correct. Sorry about that.

>
>> 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.
>
OK, good point, I should be using my bzr revision control.

> This method can be better written like this:
>
>      def __str__(self):
>          return "%2d" % self.value
>
I was switching between the 2 implementations of __str__ when I was 
testing some new code, but now need. I will just make it concrete and 
then directly request any other values.
>
>> 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:
>
I don't need them. Just hadn't settled on the order of them. But I will 
eliminate the requirement for the keyword arguments.

>          # 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.
>
Much better. I think I need to do better at breaking down the given 
tasks to their smallest chunks to best take advantage of functions.
>
>>      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)
>
Yes, another example of me trying to have my functions do too much. 
Thanks I will break it up.

Thanks again for the help, it should teach me to write better code in 
the future.