connect four (game)

[Chris Angelico]

On Sat, Nov 25, 2017 at 2:33 AM,  <namenobodywants at gmail.com> wrote:
> hi all
>
> i've just finished my first excursion into artificial intelligence with a game less trivial than tictactoe, and here it is in case anybody can offer criticism/suggestions/etc
>

Hi!

You don't have a lot of comments or docstrings or anything, so it's
not going to be easy to track down bugs. (I haven't tested your code,
so in theory it could be 100% bug-free, but frankly I doubt it. No
code is ever perfect :) )

> class infinity:
>     def __init__(self,signum): self.signum = signum
>     def __repr__(self):     return '+oo' if self.signum > 0 else '-oo'
>     def __lt__(self,other): return False if self.signum > 0 else True
>     def __le__(self,other): return False if self.signum > 0 else True
>     def __gt__(self,other): return True  if self.signum > 0 else False
>     def __ge__(self,other): return True  if self.signum > 0 else False
>     def __eq__(self,other): return isinstance(other,infinity) and self.signum == other.signum

In Python, it's normal to name classes with a capital first letter, so
"class Infinity" would be more normal.

Instead of "True if some_condition else False", you can simply write
"some_condition", and instead of "False if some_condition else True",
you can write "not some_condition".

> def getlines(board):
>     horizontal = [[(i,   j+k) for k in range(4)] for i in range(6) for j in range(7)]
>     vertical   = [[(i+k, j)   for k in range(4)] for i in range(6) for j in range(7)]
>     positive   = [[(i+k, j+k) for k in range(4)] for i in range(6) for j in range(7)]
>     negative   = [[(i+k, j-k) for k in range(4)] for i in range(6) for j in range(7)]
>     linear     = horizontal + vertical + positive + negative
>     lines      = [line for line in linear if all(i in range(6) and j in range(7) for (i,j) in line)]
>     return [[board[square] for square in line] for line in lines]

Lining up your code vertically just looks ugly if you change to a
different font. I wouldn't bother, personally.

This is the kind of function that needs a docstring and some comments.
What exactly is this doing? What are the "lines" of the board? What's
the difference between "linear" and "lines"? What exactly is it
returning?

> def getwinner(board):
>     lines = getlines(board)
>     return ex if [ex]*4 in lines else oh if [oh]*4 in lines else None

You seem to have fallen in love with the ternary 'if' expression.
While it's normal for young lovers to shower each other with
affection, it tends to get awkward for those watching you. I would
recommend breaking some of these into straight-forward 'if'
statements.

> def getmover(board):
>     boardvalues = list(board.values())
>     return ex if boardvalues.count(ex)==boardvalues.count(oh) else oh

This is a very expensive way to avoid maintaining a "whose turn is it,
anyway?" variable.

> def getoptimum(mover):
>     return max if mover == ex else min

I *think* this might be a critical part of your algorithm, but again,
in the absence of explanatory information, I can't be sure. Are you
following a basic minimax algorithm, where you assume that your
opponent will always play optimally, and you attempt to minimize your
potential losses? If so, definitely needs to be stated somewhere.
(Also, if that's the case, your code is probably locked down to having
the AI always playing the same side. Again, that needs to be stated
clearly.)

> def getvalue(winner,accessibles):
>     return infinity(+1) if winner == ex else infinity(-1) if winner == oh else 0 if not accessibles else None

Uhhhhh.... I don't even know what this is doing. Value of what? Why so
much if/else in a single expression?

> def heuristicvalue(board,depth):
>     winner      = getwinner(board)
>     accessibles = getaccessibles(board)
>     value       = getvalue(winner,accessibles)
>     if value != None: return value
>     if depth == 0:    return guessvalue(board)
>     mover    = getmover(board)
>     optimum  = getoptimum(mover)
>     children = [makemove(board,column,mover) for column in accessibles]
>     return optimum(heuristicvalue(child,depth-1) for child in children)

Anything with "heuristic" in the name is probably an important part of
your algorithm. But I've looked at the code and I don't understand
what it's doing.

> def getmoves(board,depth):
>     accessibles = getaccessibles(board)
>     if not accessibles: return []
>     mover     = getmover(board)
>     optimum   = getoptimum(mover)
>     children  = [makemove(board,column,mover) for column in accessibles]
>     values    = [heuristicvalue(child,depth)  for child  in children]
>     bestvalue = optimum(values)
>     return [accessibles[index] for index in range(len(accessibles)) if values[index] == bestvalue]

And this looks very similar to the previous one. Or at least, there
are several identical lines. Hmm.

>         [self.grid.squarebuttons[number].config(background=self.board[divmod(number,7)]) for number in range(42)]

Hrm. If this is the whole line of code, it's a list comp that discards
its result. If so, it would be far better written as a
straight-forward loop.

General summary of all of the above comments: Assume that you have to
collaborate with another programmer, and try to make his/her life
easy. You'll appreciate it six months from now, by which time you will
yourself be a different programmer, with no memory of what you were
doing as you wrote this :)

All the best!

ChrisA