Connect 4 refine the diagonal check

Question

I am creating a connect 4 game in python and I have managed to write the code so that it runs. However I have a lot of duplicated data in it and was just wondering if anyone could help me to refine it to minimise this?

Here is my code so far:

def CreateBoard(r, c, b, n):

  for i in range(1,(r+1)):
    n.append(str(i))
  b.append(n)

  for i in range(c):
    b.append(['*']*(r))
  return(b)


"""The Following PrintBoard function takes the parameter board(b), prints
the board and then returns the board."""

def printBoard(b):
  for lst in b:
    print(" ".join(lst))
  return b


"""The following Check function takes the parameter board. Within this  """
def check(board, n):
  n = []
  for i in range(1,len(board[1])+1):
    if board[1][i-1] == '*':
      n.append(i)
  print(n)

  user = (input('Enter column: '))

  if(user.isdigit() == True):
    user = int(user)
    if (user in n):
      return(user)
  print('Invalid input')
  return check(board, n)

def WinningCon(b, r, u, c):
  loop1 = True
  rowCon = ""
  colCon = ""
  for i in range(0,len(b[1])):
    rowCon += b[r][i]
    if('X'*4 in rowCon) or('O'*4 in rowCon):
      return(True)
  for i in range(1,c+1):
    colCon += b[i][u-1]
    if('X'*4 in colCon) or('O'*4 in colCon):
      return(True)


def Diag2(u, r, b, column, row):
  utemp1 = u-1
  utemp2 = u-1
  rtemp1 = r
  rtemp2 = r
  end = ""
  beg = ""
  while(True):

    beg += b[rtemp1][utemp1]
    rtemp1 -= 1
    utemp1 += 1
    if(rtemp1 == 1):
      break
    elif(utemp1 >= column - 1):
      break
  while(True):
    end += b[rtemp2][utemp2]
    rtemp2 +=1
    utemp2 -=1
    if(rtemp2 >= row):
      break
    elif(utemp2 == 0):
      break
  beg = beg[::-1]
  fullstring = beg+end
  if('X'*4 in fullstring) or('O'*4 in fullstring):
    return(True)

def Diag1(u, r, b, column, row):
  utemp1 = u-1
  utemp2 = u-1
  rtemp1 = r
  rtemp2 = r
  end = ""
  beg = ""
  while(True):
    beg += b[rtemp1][utemp1]
    rtemp1 -= 1
    utemp1 -= 1
    if(rtemp1 == 1):
      break
    elif(utemp1 == 0):
      break
  while(True):    
    end += b[rtemp2][utemp2]
    rtemp2 +=1
    utemp2 +=1
    if(rtemp2 >= row):
      break
    elif(utemp2 >= column-1):
      break
  beg = beg[::-1]
  fullstring = beg+end
  if('X'*4 in fullstring) or('O'*4 in fullstring):
    return(True)

def ProcessInput(u, s, b, r):
  rowNum = r
  u = u-1
  while(not b[rowNum][u] == '*'):
    rowNum -= 1
  b[rowNum][u] = s
  return(rowNum)

def EndGame(g, b, p):
  printBoard(b)
  print("Congrats %s, you've won!" %p)
  replayGame = input('Would you like to play again? Yes or No?\n').lower()
  if replayGame == 'yes':
    MainGame()
  else:
    g = False
    return(g)

def MainGame():
  column = int(input('Enter number of columns'))
  row = int(input('Enter number of rows'))
  board = []
  nums = []
  board = CreateBoard(column, row, board, nums)
  player1 = 'K'#input('Enter name: ')
  player2 = 'A'#input('Enter name: ')
  turn = 1
  GameOn = True
  while(GameOn):
    board = printBoard(board)
    user = check(board, nums)
    if(turn%2 != 0):
      print("It's %s's turn" %player1)
      rc = ProcessInput(user, "X", board, row)
    else:
      print("It's %s's turn" %player2)
      rc = ProcessInput(user, "O", board, row)

    if(WinningCon(board, rc , user, row) == True):
      GameOn = EndGame(GameOn, board, player1)
    elif(Diag1(user, rc, board, column, row) == True):
      GameOn = EndGame(GameOn, board, player1)
    elif(Diag2(user, rc, board, column, row) == True):
      GameOn = EndGame(GameOn, board, player1)

    turn +=1


MainGame()

Answer 1

Severe code readability issues

Honestly, your code is extremely difficult to follow and I gave up. The most important part about code is readability. Readable code is easier to reason about, easier to maintain, easier to debug, and, especially relevant here, easier to get other people's opinions on. I would very much like to give you advice on how to improve your functions - but I can't. I have no idea what they're doing now. They're simply inscrutable.

It also isn't enough that you understand what this code does right now. What if you need to go back to this code in a week? In 3 months? In a year? Will you remember what it does then? Even well-commented code with good names sometimes becomes hard to understand once you forget the context of when it was written. Poorly named, uncommented code is just setting yourself up for lots of confusion and frustration. At best.

I cannot stress this enough. Read up on the python style guide, especially for its conventions for naming and comments.

---

As far as examples of the above. We have:

def CreateBoard(r, c, b, n):

What are r, c, b, and n? I can't even tell from the body of the function. Avoid single-letter variable names unless used in a context where they are truly trivial (e.g. iterating over a range with i or _ is fine).

Function names should be snake_case, not CamelCase, and should describe what the function actually does. Certainly Diag1 and Diag2 are not helpful at all. Which one checks which kind of diagonal? What arguments do they take? What do they return?

docstrings belong inside the function:

def print_board(b):   
    """This function takes the parameter board(b), prints
    the board and then returns the board."""

When you need a comment to describe what your variable name names, that's a good indicator that you should just change your variable name. Once you do that, that doc string becomes pointless:

def print_board(board):
    ...

It's pretty clear that a function called print_board that takes a board will print it. And if it doesn't do that, then it should be renamed. somewhere.

The meaning of this docstring:

"""The following Check function takes the parameter board. Within this  """
def check(board, n):

eludes me completely. Also check suggests that we're checking something, but check is doing all sorts of things, involving taking user input. It also takes an argument n which it immediately shadows internally, and it's unclear if that's intentional or not.

WinningCon either explicitly returns True or implicitly returns None - it gives no indication as to who won. It has a variable (loop1) that isn't used.

I would strongly, strongly suggest rewriting your code with PEP-8 in mind, providing better variable names and comments, and using 4-space indentation throughout. Once you do that, you can repost and I will hopefully be able to provide more specific comments as to the actual functionality.

Answer 2

I'd like to stress @Barry's answer. It is very hard to understand what is going on in this code. You do not need to completely rewrite the whole algorithm, only help us understand what you code is doing. And the first step for that is to properly follow conventions, especially naming ones.

Your Diag1 and Diag2 functions have both the following parameters:

• u: the column where the user put its token
• r: the row where the token ended
• b: the complete board
• column: the length of the board
• row: the height of the board

Of these parameters the 2 useless ones are the one that deserved best treatment: a meaningful name. For the other ones:

• How does u relates to a column? It should be either column or column_idx.
• I spent 10 minutes understanding where did r came from; at first I thought it was the symbol of the current player ('X' or 'O'). Naming it row_idx would have helped.
• b is pretty consistent along your code, but there is no harm in calling it board.

For the useless ones, column and row can easily be computed from b (or board) using the builtin len().

Now to remove redundancy in your code, you have to look at the differences in the code. There are 3 of them:

• utemp1 += 1 in Diag2 and utemp1 -= 1 in Diag1;
• utemp2 -=1 in Diag2 and utemp2 +=1 in Diag1;
• the bounds in your elifs.

All three of them represent the same thing: the direction in which to go. You can thus use only one Diag (well, diag would be better, check_diagonal_connection even better) function which would take an extra parameter: direction. This parameter should be either 1 or -1. You would only have utemp1 += direction, utemp2 -= direction and compute the bounds for the elifs accordingly.

But… You don't even need that. You are already checking two directions in both Diag1 and Diag2. No, that's not even it, it's the same direction but you are starting from the middle. You should be able to come with a more generic approach by parametrizing your while True with:

• the row direction (up or down);
• the first slot (row/column) to check for on the board (init);
• the last slot (row/column) to check for on the board (limit).

To compute the limits, you just need simple arithmetics.

This could become something along the lines of:

def check_diagonal_connection(column_idx, row_idx, board):
    rows = len(board) # or len(board) - 1, I don't fully understand what board contains
    columns = len(board[0])

    up_init = min(column_idx, row_idx)
    up_limit = min(columns - column_idx, rows - row_idx)
    down_init = min(column_idx, rows - row_idx)
    down_limit = min(columns - column_idx, row_idx)

    directions = [
        (1, column_idx - up_init, row_idx - up_init, column_idx + up_limit, row_idx + up_limit),
        (-1, column_idx - down_init, row_idx - down_init, column_idx + down_limit, row_idx + down_limit),
    ]

    for row_dir, col_start, row_start, col_limit, row_limit in directions:
        row = row_start
        pattern = ""

        for col in range(col_start, col_limit):
            pattern += board[row][col]
            row += row_dir
            if row == row_limit:
                break

        if pattern == 'XXXX' or pattern == 'OOOO': # No need to build the strings each time
            return True # return early if we can

    return False # No 'XXXX' or 'OOOO' found

You might need to adjust these loops limits with factors of + 1 to access the last row/columns…