3
\$\begingroup\$

I started learning Python a few weeks ago and coded this "Connect 4" game.

What could I have done better? How efficient is this code and how could I improve on that?

from collections import defaultdict
from termcolor import colored
from time import sleep

class Board:
    def __init__(self):
        self.symbol = 0
        # board MAIN horizontal
        self.row1 = [0, 0, 0, 0, 0, 0, 0]
        self.row2 = [0, 0, 0, 0, 0, 0, 0]
        self.row3 = [0, 0, 0, 0, 0, 0, 0]
        self.row4 = [0, 0, 0, 0, 0, 0, 0]
        self.row5 = [0, 0, 0, 0, 0, 0, 0]
        self.row6 = [0, 0, 0, 0, 0, 0, 0]
        self.board_state_row = []
        self.board_state_collumn = []
        self.board_state_diagonal = []

    def drop_stones(self):
        l1 = [i for i, v in enumerate(self.row2) if v == 0]
        for i in l1:
            self.row2[i] = self.row1[i]
            self.row1[i] = 0
        l2 = [i for i, v in enumerate(self.row3) if v == 0]
        for i in l2:
            self.row3[i] = self.row2[i]
            self.row2[i] = 0
        l3 = [i for i, v in enumerate(self.row4) if v == 0]
        for i in l3:
            self.row4[i] = self.row3[i]
            self.row3[i] = 0
        l4 = [i for i, v in enumerate(self.row5) if v == 0]
        for i in l4:
            self.row5[i] = self.row4[i]
            self.row4[i] = 0
        l5 = [i for i, v in enumerate(self.row6) if v == 0]
        for i in l5:
            self.row6[i] = self.row5[i]
            self.row5[i] = 0

    def update(self):
        # board horizontal
        self.board_state_row = [self.row1, self.row2, self.row3, self.row4, self.row5, self.row6]
        # board vertical
        self.board_state_collumn = []
        for i in range(0, 7, 1):
            self.board_state_collumn.append(self.rows_to_collumns(i))
        # board diagonal
        rows = 6
        collumns = 7
        diagonal1 = defaultdict(list)  # For the top right to bottom left
        diagonal2 = defaultdict(list)  # For the top left to bottom right
        for i in range(rows):
            for j in range(collumns):
                diagonal1[i - j].append(self.board_state_row[i][j])
                diagonal2[i + j].append(self.board_state_row[i][j])
        self.board_state_diagonal = []
        self.board_state_diagonal.insert(0, diagonal1)
        self.board_state_diagonal.insert(1, diagonal2)

    def make_turn(self, slot, activeplayer):
        if activeplayer == 1:
            self.symbol = 1
        if activeplayer == 2:
            self.symbol = -1
        if slot in range(0, 7):
            self.row1[slot] = self.symbol
            return True
        else:
            return False

    def print_board(self):

        print(
            ' 1 ', ' | ',
            ' 2 ', ' | ',
            ' 3 ', ' | ',
            ' 4 ', ' | ',
            ' 5 ', ' | ',
            ' 6 ', ' | ',
            ' 7 ', ' | ',)

        # row1
        print(
            self.state_to_sign(self.row1[0]), ' | ',
            self.state_to_sign(self.row1[1]), ' | ',
            self.state_to_sign(self.row1[2]), ' | ',
            self.state_to_sign(self.row1[3]), ' | ',
            self.state_to_sign(self.row1[4]), ' | ',
            self.state_to_sign(self.row1[5]), ' | ',
            self.state_to_sign(self.row1[6]), ' | ')

        # row2
        print(
            self.state_to_sign(self.row2[0]), ' | ',
            self.state_to_sign(self.row2[1]), ' | ',
            self.state_to_sign(self.row2[2]), ' | ',
            self.state_to_sign(self.row2[3]), ' | ',
            self.state_to_sign(self.row2[4]), ' | ',
            self.state_to_sign(self.row2[5]), ' | ',
            self.state_to_sign(self.row2[6]), ' | ')

        # row3
        print(
            self.state_to_sign(self.row3[0]), ' | ',
            self.state_to_sign(self.row3[1]), ' | ',
            self.state_to_sign(self.row3[2]), ' | ',
            self.state_to_sign(self.row3[3]), ' | ',
            self.state_to_sign(self.row3[4]), ' | ',
            self.state_to_sign(self.row3[5]), ' | ',
            self.state_to_sign(self.row3[6]), ' | ')

        # row4
        print(
            self.state_to_sign(self.row4[0]), ' | ',
            self.state_to_sign(self.row4[1]), ' | ',
            self.state_to_sign(self.row4[2]), ' | ',
            self.state_to_sign(self.row4[3]), ' | ',
            self.state_to_sign(self.row4[4]), ' | ',
            self.state_to_sign(self.row4[5]), ' | ',
            self.state_to_sign(self.row4[6]), ' | ')

        # row5
        print(
            self.state_to_sign(self.row5[0]), ' | ',
            self.state_to_sign(self.row5[1]), ' | ',
            self.state_to_sign(self.row5[2]), ' | ',
            self.state_to_sign(self.row5[3]), ' | ',
            self.state_to_sign(self.row5[4]), ' | ',
            self.state_to_sign(self.row5[5]), ' | ',
            self.state_to_sign(self.row5[6]), ' | ')

        # row6
        print(
            self.state_to_sign(self.row6[0]), ' | ',
            self.state_to_sign(self.row6[1]), ' | ',
            self.state_to_sign(self.row6[2]), ' | ',
            self.state_to_sign(self.row6[3]), ' | ',
            self.state_to_sign(self.row6[4]), ' | ',
            self.state_to_sign(self.row6[5]), ' | ',
            self.state_to_sign(self.row6[6]), ' | ')

    def rows_to_collumns(self, index):
        collumn = []
        for i in self.board_state_row:
            collumn.append(i[index])
        return collumn

    def check_win_horizontal(self):
        for i in range(0, 6, 1):
            wincounter = 0
            for j in self.board_state_row[i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True
                    else:
                        wincounter = 0

    def check_win_vertical(self):
        for i in range(0, 7, 1):
            wincounter = 0
            for j in self.board_state_collumn[i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True

    def check_win_diagonal(self):
        for i in range(-6, 5, 1):
            wincounter = 0
            for j in self.board_state_diagonal[0][i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True
                    else:
                        wincounter = 0

        for i in range(0, 11, 1):
            wincounter = 0
            for j in self.board_state_diagonal[1][i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True
                    else:
                        wincounter = 0

    @staticmethod
    def state_to_sign(state):
        if state == 1:
            return colored(' @ ','red')
        if state == -1:
            return colored(' @ ','yellow')
        if state == 0:
            return '   '


class Player:
    def __init__(self, player_number):
        self.name = 'default'
        self.player_number = player_number

    def set_name(self, name):
        self.name = name


def start_game():
    player1 = Player(1)
    player2 = Player(2)
    board = Board()

    player1.set_name(input('Whats your name Player1?: '))
    player2.set_name(input('Whats your name Player2?: '))

    activeplayer = 1
    while activeplayer > 0:
        try:
            board.drop_stones()

            while activeplayer == 1:
                board.print_board()
                slot = int(input('Which slot do you choose ' + player1.name + '? 1-7 or 0 to exit: '))
                if slot == 0:
                    exit()
                if slot > 0:
                    slot = slot - 1
                    if board.row1[slot] != 0:
                        print('Slot full! Pick another one 1-7: ')
                        break
                    board.make_turn(slot, activeplayer)
                    board.update()
                    board.drop_stones()
                    board.update()
                    if board.check_win_diagonal() or board.check_win_horizontal() or board.check_win_vertical():
                        board.print_board()
                        print('you win', player1.name + '!')
                        activeplayer = 0
                    if not board.check_win_diagonal() \
                            and not board.check_win_horizontal() \
                            and not board.check_win_vertical():
                        activeplayer = 2
                else:
                    print('Out of range! 1-7: ')
                    break

            while activeplayer == 2:
                board.print_board()
                slot = int(input('Which slot do you choose ' + player2.name + '? 1-7 or 0 to exit: '))
                if slot == 0:
                    exit()
                if slot > 0:
                    slot = slot - 1
                    if board.row1[slot] != 0:
                        print('Slot full! Pick another one 1-7: ')
                        break
                    board.make_turn(slot, activeplayer)
                    board.update()
                    board.drop_stones()
                    board.update()
                    if board.check_win_diagonal() or board.check_win_horizontal() or board.check_win_vertical():
                        board.print_board()
                        print('you win', player2.name + '!')
                        activeplayer = 0
                    if not board.check_win_diagonal() \
                            and not board.check_win_horizontal() \
                            and not board.check_win_vertical():
                        activeplayer = 1
                else:
                    print('Out of range! 1-7: ')
                    break

        except IndexError:
            print('Out of range... 1-7! ')
        except ValueError:
            print('Only numbers! 1-7: ')

    sleep(1.5)


start_game()
\$\endgroup\$
2
\$\begingroup\$

Cool program!

Computers are machines for automation; any time you find your code is repetitive, like in print_board or the two players in start_game, it's very likely you can make the code shorter by using an array or a function.

A few other odds and ends:

  • The try block in start_game is quite long, and catches IndexError and ValueError, errors that very often indicate a bug. So if there is a bug in any of that code that causes an IndexError, the program would just print "Out of range... 1-7!" and keep going. The error message would be lost.

  • It's considered good style in Python to separate words in variable names with _, so active_player rather than activeplayer.

  • It's also considered good style for a method like check_win_horizontal to return False at the end, so that it always returns either True or False. If you don't return anything at the end of a function, Python returns None by default, so the program still works. But it's just a little confusing.

  • Your program doesn't check for the possibility of a tie game. It's possible to fill up the entire grid without anyone winning.

  • The printed board seems to need a | and some more space on the left.

  • The English word "columns" only has one L in it.

But all of this is very minor. It's a fine program. Hope you're having fun with Python! :)

\$\endgroup\$
2
\$\begingroup\$

There are a lot of issues with your code. I will try to cover all of them.

Logic Repetiton

  • In your start_game() function, you are repeating the almost same loop for both players.

You can make a current_player and use it instead of player1 or player2.

  • You can just use a else statement in this piece of code -
if board.check_win_diagonal() or board.check_win_horizontal() or 
          board.check_win_vertical():
    ...
if not board.check_win_diagonal() \
        and not board.check_win_horizontal() \
        and not board.check_win_vertical():
    activeplayer = 1

Instead -

if board.check_win_diagonal() or board.check_win_horizontal() or 
          board.check_win_vertical():
    ...
else:
    activeplayer = 1

Unnecessary objects

  • I would prefer to use a grid variable storing all the rows rather than making each row a seperate variable. Then you can iterate over each row with a loop.

  • Your class Player does not do much. You could just use player1 and player2 variables. Also you are never using Player.player_number.

Print board function

  • You can make your function shorter by using a for loop.
def print_board(self):
    print('|  1  |  2  |  3  |  4  |  5  |  6  |  7  |')
        for row in self.grid:
            print('|  ', end='')
            for index in range(7):
                print(str(self.state_to_sign(row[index])) + '|  ', end='')
            print('')

Ignoring Performance

  • Your drop_stones() function checks all columns. This affects the performance. Instead check only the column chosen by the user.

Other improvements(do not matter much)

  • By default the step parameter of range is 1. You do not need to specify it. range(1, 10) instead of range(1, 10, 1).

Happy Coding!

Edit:

This is the review for the optimized code posted again by OP.

Coupling

"How easy is it to cut out a piece of code?" If the answer is "very difficult" then our system is likely tightly coupled. Ideally, it is easy to remove code (say we find it doesn't work as intended and need to replace it) and it is easy to run that cut out code by itself (say we want to test it).

Most of the functions look good. There are some that could be improved.

def check_tie(self):
    if 0 not in self.board_state_row[0]:
        print('You tied! No one Wins!')
        sleep(1.5)
        exit()

What if you wanted to test this function?(I know its very simple function)

You wont be able to check it multiple times because it will end the program.

You want to only return if its a tie or not and do the necessary outside the function.

def check_tie(self):
    return 0 not in self.board_state_row[0]

Credit for the explanation to spyr03

List Comprehension

  • When ever you can, you should use a list comprehension as it almost always faster than for loops. So you can upgrade your columns_to_rows() accordingly.

if name == 'main'

  • You should always use if __name__ == '__main__' in your files.

You should wrap your all all your while active_player > 0 code in a function play() and then use it under the if __name__ == '__main__' clause

if __name__ == '__main__':
    play()

You can read more about it on this Stack Overflow answer.

F Strings

  • You can use f-strings from Python 3.6. f-strings are simply faster than %s and str.format().

Other Improvement

  • In this piece of code:
if active_player == 1:
    return int(2)
if active_player == 2:
    return int(1)

You can just return active_player.

  • When defining matrices, it is nice to define it like this:
matrix = [
    [0, 0, 0, 0],
    [0, 0, 0, 0],
    [0, 0, 0, 0],
    [0, 0, 0, 0]
]

It also is easier to understand its structure.

  • You can use the += operator to perform some of your calculations.

Example - a = a + 1 is same as a += 1.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much, i will look into everything you mentioned and try to improve things :) \$\endgroup\$ Sep 21 at 18:33
1
\$\begingroup\$

So finally, with your super helpful tips and after a few hours of improving stuff I got rid of a few lines of code, all the self.row variables and improved some of the functions. Also I got rid of the Player class since it was only storing the names, which I implemented as variables now.

The drop_stones and print_board methods are also much less code now and drop_stones now uses the selected column instead of every row so it has to check and change only one list instead of a whole 2d list. Next thing on the list would be a function to check for a tie, but I'm sure I need another few hours to come up with a crappy solution for that XD

Edit: I wrote the check_tie function to check for a tie (when the first of the row is full and no one has won). Also I fixed a bug where input was declared as str while int was needed.

Here is the "optimized" code:

from collections import defaultdict, deque
from termcolor import colored
from time import sleep


class Board:
    def __init__(self):
        self.symbol = 0
        self.board_state_row = [[0,0,0,0,0,0,0], [0,0,0,0,0,0,0], [0,0,0,0,0,0,0], [0,0,0,0,0,0,0], [0,0,0,0,0,0,0], [0,0,0,0,0,0,0]]
        self.board_state_column = [[0,0,0,0,0,0],[0,0,0,0,0,0],[0,0,0,0,0,0],[0,0,0,0,0,0],[0,0,0,0,0,0],[0,0,0,0,0,0],[0,0,0,0,0,0]]
        self.board_state_diagonal = []

    def drop_stones(self, slot):
        a = self.board_state_column[slot]
        b = deque([x for x in a if x != 0])
        for i in a:
            if i == 0:
                b.appendleft(0)
        self.board_state_column[slot] = b

    def update(self):
        # board rows
        self.board_state_row = []
        for i in range(6):
            self.board_state_row.append(self.columns_to_rows(i))

        # board diagonals
        rows = 6
        columns = 7
        diagonal1 = defaultdict(list)  # For the top right to bottom left
        diagonal2 = defaultdict(list)  # For the top left to bottom right
        for i in range(rows):
            for j in range(columns):
                diagonal1[i - j].append(self.board_state_row[i][j])
                diagonal2[i + j].append(self.board_state_row[i][j])
        self.board_state_diagonal = []
        self.board_state_diagonal.insert(0, diagonal1)
        self.board_state_diagonal.insert(1, diagonal2)

    def make_turn(self, slot, active_player):
        if active_player == 1:
            self.symbol = 1
        if active_player == 2:
            self.symbol = -1
        if slot in range(0, 7):
            self.board_state_column[slot][0] = self.symbol
            return True
        else:
            return False

    def print_board(self):
        for i in range(1,8):
            print(' '+str(i)+' ', ' | ', end= ' ')
        print('')

        for i in range(6):
            for j in range(7):
                print(self.state_to_sign(self.board_state_row[i][j]), ' | ',end=' ')
            print('')

    def columns_to_rows(self, index):
        row = []
        for i in self.board_state_column:
            row.append(i[index])
        return row

    def check_win_horizontal(self):
        for i in range(6):
            wincounter = 0
            for j in self.board_state_row[i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True
                    else:
                        wincounter = 0

        if wincounter == 0:
            return False

    def check_win_vertical(self):
        for i in range(7):
            wincounter = 0
            for j in self.board_state_column[i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True

        if wincounter == 0:
            return False

    def check_win_diagonal(self):
        for i in range(-6, 5):
            wincounter = 0
            for j in self.board_state_diagonal[0][i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True
                    else:
                        wincounter = 0

        for i in range(11):
            wincounter = 0
            for j in self.board_state_diagonal[1][i]:
                if j != self.symbol:
                    wincounter = 0
                if j == self.symbol or wincounter == 0:
                    if j == self.symbol:
                        wincounter = wincounter + 1
                        if wincounter == 4:
                            return True
                    else:
                        wincounter = 0

        if wincounter == 0:
            return False

    def check_tie(self):
        if 0 not in self.board_state_row[0]:
            print('You tied! No one Wins!')
            sleep(1.5)
            exit()

    @staticmethod
    def state_to_sign(state):
        if state == 1:
            return colored(' @ ','red')
        if state == -1:
            return colored(' @ ','yellow')
        if state == 0:
            return '   '


def game(current_player,active_player):
    board.print_board()
    slot = int(input('Which slot do you choose ' + current_player + '? 1-7 or 0 to exit: '))
    if slot == 0:
        exit('Exit by selection.')
    if slot > 0:
        slot = slot - 1
        while board.board_state_row[0][slot] != 0:
            slot = int(input('Slot full! Pick another one 1-7: '))
            slot = slot -1

        board.make_turn(slot, active_player)
        board.update()
        board.drop_stones(slot)
        board.update()
        if board.check_win_diagonal() or board.check_win_horizontal() or board.check_win_vertical():
            board.print_board()
            print('you win '+ current_player + '!')
            sleep(1.5)
            exit(0)
        board.check_tie()
        if active_player == 1:
            return int(2)
        if active_player == 2:
            return int(1)


board = Board()
player1 = input('Whats your name Player1?: ')
player2 = input('Whats your name Player2?: ')
active_player = 1

while active_player > 0:
    try:
        while active_player == 1:
            active_player = game(player1, active_player)

        while active_player == 2:
            active_player = game(player2, active_player)

    except IndexError:
        print('Out of range... 1-7! ')
    except ValueError:
        print('Only numbers! 1-7: ')
sleep(1.5)
\$\endgroup\$
1
  • \$\begingroup\$ I edited my answer to higlight some issues in your code. Have a look. \$\endgroup\$ Sep 23 at 6:48

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.