10
\$\begingroup\$

I am currently using Python 3.5.2 to create a connect four basic game. My game should work for two players and tell you when you have won, be it horizontal vertical or diagonal. However I do not know how to make my code smaller or more efficient, is there a way to cut down, or is my method just not very efficient in itself?

I have tried looking up various other ways of doing this code, alas I found them difficult to understand. My code does not have any errors I have picked up on, and though I have completed the task I set myself, I do not feel this is the best way to do it.

If anybody had any suggestions, that would be much appreciated. If I knew how to make it shorter without losing functionality I would do so.

I hope my code will help anybody else attempting the same task as mine is rather basic.

import time    

grid1 = [0,0,0,0] # bottom row
grid2 = [0,0,0,0]
grid3 = [0,0,0,0]
grid4 = [0,0,0,0]

grids = [grid1,grid2,grid3,grid4]

check = []

user = 1

class fullSlot_error (Exception):
    pass
def hasWon_def():
    print ("player "+str(user)+" has won")
    time.sleep (1)

def grid_def():
    print ("",grid4,"\n",grid3,"\n",grid2,"\n",grid1)

def user_def():
    global user
    if user < 2:
        user = 2
    else:
        user = 1
    return user

def slotFull_def():
   while True:
        try:
            if grid4[userInput -1] != 0:
                raise fullSlot_error
            else:
                break
        except fullSlot_error:
            print ("slot is full try again")
            confirm_def()

def confirm_def():
    looop= True
    while looop== True:
        try:
            global userInput
            userInput = int(input("\ninput a slot player "+str(user)+"(1,4)\n"))
            if userInput < 5 and 0 < userInput:   
                looop = False
            else:
                print ("invalid int")
        except ValueError:
            print ("invalid input")

def placement_def():
    counter = 0
    for i in range (0,4):
        slotFull_def()
        if (grids[i][userInput -1] == 0):
            grids [i][userInput - 1] = int(user)
            grid_def()
            break


def check_def():
    global loop
    global check
    for i in range(0,4):
        for a in range(0,4):
            check.append(grids[i][a])
        if (check == [1,1,1,1] or check == [2,2,2,2]):
            hasWon_def()
            loop = False
            return loop
            break
        else:
            check = []
    for i in range(0,4):
        for a in range(0,4):
            check.append(grids[a][i])
        if (check == [1,1,1,1] or check == [2,2,2,2]):
            hasWon_def()
            loop = False
            return loop
            break
        else:
            check = []

def checkEmpty_def():
    global check
    for i in range (0,4):
        for a in range (0,4):
            check.append(grids[i][a])
    if 0 not in check:
        print ("full")

def checks_def():
    check_def()
    checkEmpty_def()
    diagcheck_def()

def diagcheck_def():
    global loop
    global check
    check = []
    diag = 0
    for i in range (0,4):
        check.append (grids[diag][diag])
        diag = diag +1
        if (check == [1,1,1,1] or check == [2,2,2,2]):
            hasWon_def()
            loop = False
            return loop
            break
    check = []
    diag = 3
    diag2 = 0
    for i in range (0,4):
        check.append (grids[diag][diag2])
        if (check == [1,1,1,1] or check == [2,2,2,2]):
            hasWon_def()
            loop = False
            return loop
            break


loop = True

while loop == True:
    check_def()
    confirm_def()
    placement_def()
    checks_def()
    if loop == False:
        break
    user_def()
\$\endgroup\$

1 Answer 1

11
\$\begingroup\$

Some suggestions:

  1. It is generally recommended to follow the pep8 style for Python code. For example no space between function names and paranthesis, lower_case_function_names, spaces after commas, etc.
  2. You can define your grid in a single operation rather than 5.
  3. For print, you can use * to unpack your nested list, then use sep='\n' to put each row on a different line.
  4. It would be better to have at least one top-level function, rather than doing everything in the root of the script.
  5. You don't need global variables, it is better to pass the variables as arguments and return the result.
  6. For print, if you give multiple values as arguments, spaces are automatically inserted between the arguments.
  7. You don't get any benefit from using a custom exception here, just use a break in your if test.
  8. you can use one-line if tests (called "ternary expressions") to simplify some of your code.
  9. There isn't much benefit from a simple one-line function that is only used in one place. It makes things harder to read and slows the code down a tiny bit. Just inline the code.
  10. There isn't much point using a sentinel variable (like looop) when you just assign False to it at a certain point. Just use break, and continue if need be to skip later parts of the loop.
  11. It is better to put the absolute minimal amount of code possible in your try block to avoid accidentally catching the wrong exception.
  12. you can combine multiple comparisons, such as 0 < userInput < 5.
  13. You can use return instead of break to exit out of a while loop if you just want to return the result at that point with no further processing.
  14. You can use in to test if a value is in a list or other sequence of values rather then using and.
  15. You never use counter in placement_def.
  16. The 0 is implied in range(0, x), so you can leave it out.
  17. Rather than using range and indexing a list, it is easier to iterate over the list directly.
  18. You always subtract 1 from userInput. It would be easier to subtract one at the beginning.
  19. Rather than appending a list several times to another list, use extend to append the all the elements at once.
  20. check doesn't need to be global, its state is never shared between functions.
  21. You can use all to simplify checking rows or columns, and zip to transpose lists.
  22. You can use any and all to simplify your checks for whether a player has won and for whether the grid is full.
  23. You could simplify the code somewhat by having the check functions return a boolean, then printing in checks_def.
  24. I don't think placement_def is correct. You get a user input every time through the loop, rather than just once, and you overwrite the value if it is the current use or empty, when you want to overwrite only if empty. Further, you loop from top to bottom, when you want to loop from bottom to top.
  25. You hard-code the grid size, but it would be easy to allow the user to specify a square grid size.

So here is my code:

def play(n=None):
    if n is None:
        while True:
            try:
                n = int(input('Input the grid size: '))
            except ValueError:
                print('Invalid input')
                continue
            if n <= 0:
                print('Invalid input')
                continue
            break

    grids = [[0]*n for _ in range(n)]
    user = 1
    print('Current board:')
    print(*grids, sep='\n')
    while True:
        user_input = get_input(user, grids, n)
        place_piece(user_input, user, grids)
        print('Current board:')
        print(*grids, sep='\n')

        if (check_won(grids, user, n) or
                check_won(zip(*grids), user, n) or
                diagcheck_won(grids, user, n) or
                diagcheck_won(grids[::-1], user, n)):
            print('Player', user, 'has won')
            return

        if not any(0 in grid for grid in grids):
            return

        user = 2 if user == 1 else 1


def get_input(user, grids, n):
    instr = 'Input a slot player {0} from 1 to {1}: '.format(user, n)
    while True:
        try:
            user_input = int(input(instr))
        except ValueError:
            print('invalid input:', user_input)
            continue
        if 0 > user_input or user_input > n+1:
            print('invalid input:', user_input)
        elif grids[0][user_input-1] != 0:
            print('slot', user_input, 'is full try again')
        else:
            return user_input-1


def place_piece(user_input, user, grids):
    for grid in grids[::-1]:
        if not grid[user_input]:
            grid[user_input] = user
            return


def check_won(grids, user, n):
    return any(all(cell == user for cell in grid) for grid in grids)


def diagcheck_won(grids, user, n):
    return all(grids[x][x] == user for x in range(n))


if __name__ == '__main__':
    play()

Note that this could be simplified even further using numpy arrays, but that is more advanced.

\$\endgroup\$
5
  • \$\begingroup\$ Could you please explain what grids [::-1] is and what its purpose is? I assume it is some sort of callback to an existing variable however being new to python, I do not know how it would do this. \$\endgroup\$ Jul 13, 2016 at 14:16
  • \$\begingroup\$ @bubblezgopop slice syntax is l[start:stop:step]. A step of -1 means going backwards through the list (start is assumed to be beginning of the list and end the end of the list if not specified, like here). \$\endgroup\$
    – Graipher
    Jul 13, 2016 at 14:24
  • \$\begingroup\$ @bubblezgopop: As Graipher said, it is going backwards. The idea is to start at the bottom of the board, and then move upwards row-by-row until it finds one where the chosen column is empty. That way the board fills bottom-to-top (as in a real connect 4 board) rather than top-to-bottom as you had. \$\endgroup\$ Jul 13, 2016 at 16:22
  • \$\begingroup\$ what if grid is of 6 x 7 ? \$\endgroup\$ Oct 12, 2017 at 5:11
  • \$\begingroup\$ @YaShChaudhary: You ask for two numbers,, say m and n. You can either get them a single input with split to separate them or two inputs. Then you use the two numbers where you previously used one, something like grids = [[0]*m for _ in range(n)]. \$\endgroup\$ Nov 1, 2017 at 19:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.