0
\$\begingroup\$

I wrote a connect four game for two players, where each player uses the same keyboard. I programmed this solely in hopes of improving my skills.

I'm open to and looking for any sort of feedback. Do the comments contain sufficient detail to understand the code? Are the functions separated into their individual mechanisms/purposes properly? Is it, overall, a decently written algorithm?

I'm especially skeptical of the check_winner function. I feel like it could be optimized, just not sure how.

import math

def board_setup():
  """Setup game board dimension size; returns game board as list"""

  board_size = -1
  while board_size < 4 or board_size > 9:
    try:
      board_size = int(input("Enter board size dimension (minimum=4, maximum=9): "))
    except ValueError:
      print("Error: Enter an integer")

  return [" "]*board_size**2

def player_setup():
  """Setup two players and their corresponding symbol/piece to represent them on the game board; returns list of player symbols/pieces"""

  players = ["too many"]*2
  for i in range(2):
    while len(players[i]) > 1:
      players[i] = input("Player " + str(i+1) + ": Enter your symbol/piece (1 character in length): ")

      if players[0] == players[1] or players[i].isspace() or players[i] == "":
        print("Error: Duplicate or unsupported player symbol provided")
        players[i] = "too many"

  return players

def line_break(lines):
  """Prints specified number of blank lines
  
  lines: number of lines"""

  print("\n"*lines)

def print_board_pieces(board):
  """Prints board's vertical and horizontal barrier and player symbols/pieces set in board
  
  board: board array"""

  board_width = int(math.sqrt(len(board)))
  barrier_symbol = "▋"

  #print player pieces or blank space and vertical barriers
  for s in range(0, len(board), board_width):
    print(barrier_symbol, *board[s:s+board_width], barrier_symbol, sep="|")
    if s < len(board)-board_width:
      print(barrier_symbol, "+", "-+"*board_width, barrier_symbol, sep="")

  #horizontal floor of board
  print(barrier_symbol, barrier_symbol*board_width*2, barrier_symbol*2, sep="")

def print_board(board, numbered):
  """Print game board to screen
  
  board: board array
  numbered: number top of columns?; bool"""

  line_break(3) #padding

  #number rows
  if numbered:
    print(" ", end="")
    for i in range(int(math.sqrt(len(board)))):
      print(" ", str(i+1), sep="", end="")
    print(" ")
  else:
    print()

  #barrier and player pieces
  print_board_pieces(board)

  line_break(3) #padding

def player_move(board, player):
  """Prompt and apply player input to board; returns index of player's movement in the board array as int
  
  board: board array to modify
  player: player symbol/piece to apply"""

  board_width = int(math.sqrt(len(board)))
  board_size = len(board)

  print("It is " + player + "'s turn!")

  move = -1
  succeeded = False
  while not succeeded:
    while move < 0 or move >= board_width:
        try:
          move = int(input("Where would you like to drop your symbol/piece (enter the integer representing the column)?: "))-1
        except ValueError:
          print("Error: Enter an integer")

    move = board_size - (board_width - move) #move symbol/piece to bottommost row of board

    #move player piece up vertically on board until spot is free
    while 0 <= move < board_size and board[move] != " ":
      move -= board_width

    #if piece is in free and valid spot on board, end root loop
    if 0 <= move < board_size:
      succeeded = True
    else:
      print("Error: No space left in column")
    
  board[move] = player #apply player piece/symbol to board
  
  return move

def switch_player_turn(player):
  """Switches current player turn; returns other player index in array as int
  
  player: player index in players list as int"""

  return 0 if player == 1 else 1

def check_winner(board, player, index):
  """Check if player has won vertically, diagonally, horizontally, if it's a tie, or if the game should continue
  
  board: board array
  player: player symbol/piece as string
  index: newly placecd player piece's/symbol's index in board array to check from"""

  board_width = board_height = int(math.sqrt(len(board)))
  board_size = len(board)

  #horizontal check
  row_start = board_width * math.floor(moved/board_width)
  if board[row_start:row_start+board_width].count(player) >= 4:
    connected = 1
    l, r = moved-1, moved+1
    while connected < 4 and r < row_start+board_width and board[r] == player:
      connected += 1
      r += 1
    while connected < 4 and l >= row_start and board[l] == player:
      connected += 1
      l -= 1
    if connected == 4:
      return player

  #vertical check
  column_start = moved
  while column_start >= board_height:
    column_start -= board_height
  if board[column_start:board_size:board_height].count(player) >= 4:
    connected = 1
    l, r = moved-board_height, moved+board_height
    while connected < 4 and r < board_size and board[r] == player:
      connected += 1
      r += board_height
    while connected < 4 and l >= 0 and board[l] == player:
      connected += 1
      l -= board_height
    if connected == 4:
      return player

  #diagonal check
  _dir = -1
  while _dir <= 1:
    diag_start = diag_end = moved
    while diag_start % board_height != 0 if _dir == 1 else (diag_start+1) % board_height != 0 and diag_start >= board_width:
      diag_start -= board_height+_dir #move diag_start as far top as possible until wall collision
    while diag_end % board_height != 0 if _dir == -1 else (diag_end+1) % board_height != 0  and diag_end < (board_size-board_width):
      diag_end += board_height+_dir #move diag_end as far bottom as possible until wall collision
    if board[diag_start:diag_end+1:board_height+_dir].count(player) >= 4:
      connected = 1
      l, r = moved-(board_height+_dir), moved+(board_height+_dir)
      while connected < 4 and r <= diag_end and board[r] == player:
        connected += 1
        r += board_height+_dir
      while connected < 4 and l >= diag_start and board[l] == player:
        connected += 1
        l -= board_height+_dir
      if connected == 4:
        return player
    _dir += 2

  #draw/tie check
  if " " not in board:
    return "No one"
  else:
    return None #no winner or draw yet

#board setup
board_arr = board_setup()

#player setup
players = player_setup()
current_player = 0

#main loop
winner = None
while winner == None:
  print_board(board_arr, numbered=True)
  moved = player_move(board_arr, players[current_player])
  winner = check_winner(board_arr, players[current_player], moved)
  if winner:
    break
  else:
    current_player = switch_player_turn(current_player)

#winner
print("\n"*100)
print_board(board_arr, numbered=False)
if winner in players:
  print(winner, "won!")
else:
  print("The board is full, it's a draw!")
\$\endgroup\$
1
\$\begingroup\$

The code is not hard to follow, I'd just recommend overall to use a matrix since it will ease all operations to check who has won

I added a suggestion for checking vertical/horizontal winning condition with your current plain list implementation

for i in range(board_width):
    # horizontal check
    row = board[i*board_width:i*board_width+board_width]
    if player*4 in ''.join(row):
        return player

    # vertical check
    column = [board[j*board_width+i] for j in range(board_width)]
    print(column)
    if player*4 in ''.join(column):
        return player

That's the biggest function that I will aim to reduce, diagonals are a bit trickier, but they get easier if you use a matrix.

I'd also check the function to move player, since it will get out of hand easily

Happy coding!

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

Overall not bad at all,

  1. You are correct with respect to the correct_winner function. Robert Martin, in his book "Clean Code" expresses in humerous terms just how small a function should be (hint, really small). When I see code that is broken up by comments (eg. #horiztontal check, it immediately suggests to me that it should be its own function. I believe this is true in your case.
  1. Use Python type hints, it will make the world a better place.

  2. Some of your function names are not meaningful enough. "print_board_pieces" is a good name because it gives me an idea of what you are doing, "player_move" not so much. Are you handling player move? Printing player move? It is unclear to me by the name.

  3. Make one mental thought per line. When you write "while connected < 4 and r < row_start+board_width and board[r] == player", that is two thoughts per line, the while statement and the logic. But I have no idea what the logic is. The following is much more readable.

    is_connected = while connected < 4 and r < row_start+board_width and board[r] == player
    while is_connected:
    ....do something

It is worth piking up Martin's book on clean code. It is an easy read and will be of great benefit to you -- it completely changed (for the better I hope) the way I write code some 15 or so years ago. Another good book is CodeCommit.

Cheers,

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