6
\$\begingroup\$

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()
\$\endgroup\$
  • \$\begingroup\$ Don't worry about posting too much code, we are quite used to it here. I hope you get some good answers! \$\endgroup\$ – Phrancis Dec 29 '15 at 15:41
  • 1
    \$\begingroup\$ I rolled back your edit - see is it okay to edit my own question to include revised code? \$\endgroup\$ – Barry Dec 29 '15 at 16:00
  • 1
    \$\begingroup\$ Posting code that you worked on collaboratively is fine, but we expect you to understand the code you posted. Code Review's purpose is to suggest improvements to your code, not to explain code to you. \$\endgroup\$ – 200_success Dec 29 '15 at 16:01
  • 2
    \$\begingroup\$ The changes you made invalidated the given answer. Answer invalidation is a big no-no on Stack Exchange sites. \$\endgroup\$ – Mast Dec 29 '15 at 16:08
  • 1
    \$\begingroup\$ Note that it's OK to post revised code in a new question, and a good option in this case since the answerers are having a difficult time with it as-is. (Make sure to make the changes the answers here suggested first, though!) \$\endgroup\$ – Veedrac Dec 29 '15 at 21:33
18
+500
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ @Brigzy97 The point is - I can't really give you meaningful comments about your diagonal checks since I have no idea what is going on in either of those functions. Their names aren't helpful (what's the diff b/t Diag1 and Diag2? Will you remember 2 months from now?) The variable names aren't helpful (what're u, r, and b?) The goal of the function is unclear. Until you rewrite the code in a way to make it clear what it's supposed to do - I can't really help you solve that problem in a better way. \$\endgroup\$ – Barry Dec 29 '15 at 15:56
  • \$\begingroup\$ @Brigzy97 I know what connect 4 is. The question was which function checks which diagonal type? And it doesn't matter what they're called from - MainGame() could be in a different file in a different directory somewhere - it's important that you can look at the function itself, by itself, and understand what it does. It's important that your colleagues can do it as easily as you. \$\endgroup\$ – Barry Dec 29 '15 at 16:09
  • \$\begingroup\$ @Barry Your review is on point, but I empathise with Brigzy97 in that it's important not to come across as abrasive. The header and first sentence are perhaps overboard. \$\endgroup\$ – Veedrac Dec 29 '15 at 21:28
  • 2
    \$\begingroup\$ @Veedrac OP has two reviews now indicating that it's impossible to understand the code. This isn't abrasive or overboard - it's hugely important to convey! If anything, it's insufficiently emphasized in both reviews. And if OP can't handle this kind of criticism, OP shouldn't be asking for it. \$\endgroup\$ – Barry Dec 29 '15 at 21:42
  • 2
    \$\begingroup\$ This answer is the winner of Best of Code Review 2015 — Diplomat category. Congratulations! \$\endgroup\$ – 200_success Jan 16 '16 at 0:50
10
\$\begingroup\$

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…

| improve this answer | |
\$\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.