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()
1 Answer 1
Some suggestions:
- 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. - You can define your grid in a single operation rather than 5.
- For
print
, you can use*
to unpack your nested list, then usesep='\n'
to put each row on a different line. - It would be better to have at least one top-level function, rather than doing everything in the root of the script.
- You don't need global variables, it is better to pass the variables as arguments and return the result.
- For print, if you give multiple values as arguments, spaces are automatically inserted between the arguments.
- You don't get any benefit from using a custom exception here, just use a
break
in yourif
test. - you can use one-line
if
tests (called "ternary expressions") to simplify some of your code. - 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.
- There isn't much point using a sentinel variable (like
looop
) when you just assignFalse
to it at a certain point. Just usebreak
, andcontinue
if need be to skip later parts of the loop. - It is better to put the absolute minimal amount of code possible in your
try
block to avoid accidentally catching the wrong exception. - you can combine multiple comparisons, such as
0 < userInput < 5
. - You can use
return
instead ofbreak
to exit out of awhile
loop if you just want to return the result at that point with no further processing. - You can use
in
to test if a value is in a list or other sequence of values rather then usingand
. - You never use
counter
inplacement_def
. - The
0
is implied inrange(0, x)
, so you can leave it out. - Rather than using
range
and indexing a list, it is easier to iterate over the list directly. - You always subtract
1
fromuserInput
. It would be easier to subtract one at the beginning. - Rather than appending a list several times to another list, use
extend
to append the all the elements at once. check
doesn't need to be global, its state is never shared between functions.- You can use
all
to simplify checking rows or columns, andzip
to transpose lists. - You can use
any
andall
to simplify your checks for whether a player has won and for whether the grid is full. - You could simplify the code somewhat by having the check functions return a boolean, then printing in
checks_def
. - 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. - 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.
-
\$\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\$bubblez go pop– bubblez go pop2016年07月13日 14:16:47 +00:00Commented 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– Graipher2016年07月13日 14:24:06 +00:00Commented 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\$TheBlackCat– TheBlackCat2016年07月13日 16:22:02 +00:00Commented Jul 13, 2016 at 16:22
-
\$\begingroup\$ what if grid is of 6 x 7 ? \$\endgroup\$YaSh Chaudhary– YaSh Chaudhary2017年10月12日 05:11:10 +00:00Commented Oct 12, 2017 at 5:11
-
\$\begingroup\$ @YaShChaudhary: You ask for two numbers,, say
m
andn
. You can either get them a singleinput
withsplit
to separate them or twoinput
s. Then you use the two numbers where you previously used one, something likegrids = [[0]*m for _ in range(n)]
. \$\endgroup\$TheBlackCat– TheBlackCat2017年11月01日 19:33:41 +00:00Commented Nov 1, 2017 at 19:33