7
\$\begingroup\$

I've made a simple connect 4 game using PyGame. I'm new to python and haven't completely perfected this. I know it's inefficient but I can't tell exactly HOW to fix it? Could someone help me out? Would be greatly appreciated.

import pygame
from colors import *
import random
pygame.init()

size = (320, 280)
screen = pygame.display.set_mode(size)
pygame.display.set_caption("Connect 4 but Worse")
done = False
clock = pygame.time.Clock()

turn = 0

class Space():

    def __init__(self):
        self.color = WHITE
        self.height = 40
        self.width = 40
        self.x = 5
        self.y = 5
    def Draw(self, screen):
        pygame.draw.ellipse(screen, self.color, [self.x, self.y, 40, 40])

x_list = [5, 50, 95, 140, 185, 230, 275]
y_list = [5, 50, 95, 140, 185, 230]

spaces_list = []
x_counter = 0
y_counter = 0

for i in range(42):
    slot = Space()
    slot.color = WHITE
    slot.height = 40
    slot.width = 40
    slot.x = x_list[x_counter]
    slot.y = y_list[y_counter]

    spaces_list.append(slot)

    x_counter += 1
    if x_counter == 7:
        y_counter +=1
        x_counter = 0

while not done:

    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            done = True
        if event.type == pygame.MOUSEBUTTONDOWN:
            (posx,posy) = pygame.mouse.get_pos()
            if posx > 5 and posx < 45:
                if spaces_list[35].color == WHITE:
                    if turn == 0:
                        spaces_list[35].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[35].color = YELLOW
                        turn = 0
                elif spaces_list[28].color == WHITE:
                    if turn == 0:
                        spaces_list[28].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[28].color = YELLOW
                        turn = 0
                elif spaces_list[21].color == WHITE:
                    if turn == 0:
                        spaces_list[21].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[21].color = YELLOW
                        turn = 0
                elif spaces_list[14].color == WHITE:
                    if turn == 0:
                        spaces_list[14].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[14].color = YELLOW
                        turn = 0
                elif spaces_list[7].color == WHITE:
                    if turn == 0:
                        spaces_list[7].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[7].color = YELLOW
                        turn = 0
                elif spaces_list[0].color == WHITE:
                    if turn == 0:
                        spaces_list[0].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[0].color = YELLOW
                        turn = 0
            if posx > 50 and posx < 90:
                if spaces_list[36].color == WHITE:
                    if turn == 0:
                        spaces_list[36].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[36].color = YELLOW
                        turn = 0
                elif spaces_list[29].color == WHITE:
                    if turn == 0:
                        spaces_list[29].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[29].color = YELLOW
                        turn = 0
                elif spaces_list[22].color == WHITE:
                    if turn == 0:
                        spaces_list[22].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[22].color = YELLOW
                        turn = 0
                elif spaces_list[15].color == WHITE:
                    if turn == 0:
                        spaces_list[15].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[15].color = YELLOW
                        turn = 0
                elif spaces_list[8].color == WHITE:
                    if turn == 0:
                        spaces_list[8].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[8].color = YELLOW
                        turn = 0
                elif spaces_list[1].color == WHITE:
                    if turn == 0:
                        spaces_list[1].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[1].color = YELLOW
                        turn = 0
            if posx > 95 and posx < 135:
                if spaces_list[37].color == WHITE:
                    if turn == 0:
                        spaces_list[37].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[37].color = YELLOW
                        turn = 0
                elif spaces_list[30].color == WHITE:
                    if turn == 0:
                        spaces_list[30].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[30].color = YELLOW
                        turn = 0
                elif spaces_list[23].color == WHITE:
                    if turn == 0:
                        spaces_list[23].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[23].color = YELLOW
                        turn = 0
                elif spaces_list[16].color == WHITE:
                    if turn == 0:
                        spaces_list[16].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[16].color = YELLOW
                        turn = 0
                elif spaces_list[9].color == WHITE:
                    if turn == 0:
                        spaces_list[9].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[9].color = YELLOW
                        turn = 0
                elif spaces_list[2].color == WHITE:
                    if turn == 0:
                        spaces_list[2].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[2].color = YELLOW
                        turn = 0
            if posx > 140 and posx < 180:
                if spaces_list[38].color == WHITE:
                    if turn == 0:
                        spaces_list[38].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[38].color = YELLOW
                        turn = 0
                elif spaces_list[31].color == WHITE:
                    if turn == 0:
                        spaces_list[31].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[31].color = YELLOW
                        turn = 0
                elif spaces_list[24].color == WHITE:
                    if turn == 0:
                        spaces_list[24].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[24].color = YELLOW
                        turn = 0
                elif spaces_list[17].color == WHITE:
                    if turn == 0:
                        spaces_list[17].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[17].color = YELLOW
                        turn = 0
                elif spaces_list[10].color == WHITE:
                    if turn == 0:
                        spaces_list[10].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[10].color = YELLOW
                        turn = 0
                elif spaces_list[3].color == WHITE:
                    if turn == 0:
                        spaces_list[3].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[3].color = YELLOW
                        turn = 0
            if posx > 185 and posx < 225:
                if spaces_list[39].color == WHITE:
                    if turn == 0:
                        spaces_list[39].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[39].color = YELLOW
                        turn = 0
                elif spaces_list[32].color == WHITE:
                    if turn == 0:
                        spaces_list[32].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[32].color = YELLOW
                        turn = 0
                elif spaces_list[25].color == WHITE:
                    if turn == 0:
                        spaces_list[25].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[25].color = YELLOW
                        turn = 0
                elif spaces_list[18].color == WHITE:
                    if turn == 0:
                        spaces_list[18].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[18].color = YELLOW
                        turn = 0
                elif spaces_list[11].color == WHITE:
                    if turn == 0:
                        spaces_list[11].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[11].color = YELLOW
                        turn = 0
                elif spaces_list[4].color == WHITE:
                    if turn == 0:
                        spaces_list[4].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[4].color = YELLOW
                        turn = 0
            if posx > 230 and posx < 270:
                if spaces_list[40].color == WHITE:
                    if turn == 0:
                        spaces_list[40].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[40].color = YELLOW
                        turn = 0
                elif spaces_list[33].color == WHITE:
                    if turn == 0:
                        spaces_list[33].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[33].color = YELLOW
                        turn = 0
                elif spaces_list[26].color == WHITE:
                    if turn == 0:
                        spaces_list[26].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[26].color = YELLOW
                        turn = 0
                elif spaces_list[19].color == WHITE:
                    if turn == 0:
                        spaces_list[19].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[19].color = YELLOW
                        turn = 0
                elif spaces_list[12].color == WHITE:
                    if turn == 0:
                        spaces_list[12].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[12].color = YELLOW
                        turn = 0
                elif spaces_list[5].color == WHITE:
                    if turn == 0:
                        spaces_list[5].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[5].color = YELLOW
                        turn = 0
            if posx > 275:
                if spaces_list[41].color == WHITE:
                    if turn == 0:
                        spaces_list[41].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[41].color = YELLOW
                        turn = 0
                elif spaces_list[34].color == WHITE:
                    if turn == 0:
                        spaces_list[34].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[34].color = YELLOW
                        turn = 0
                elif spaces_list[27].color == WHITE:
                    if turn == 0:
                        spaces_list[27].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[27].color = YELLOW
                        turn = 0
                elif spaces_list[20].color == WHITE:
                    if turn == 0:
                        spaces_list[20].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[20].color = YELLOW
                        turn = 0
                elif spaces_list[13].color == WHITE:
                    if turn == 0:
                        spaces_list[13].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[13].color = YELLOW
                        turn = 0
                elif spaces_list[6].color == WHITE:
                    if turn == 0:
                        spaces_list[6].color = RED
                        turn = 1
                    elif turn == 1:
                        spaces_list[6].color = YELLOW
                        turn = 0

    screen.fill(BLUE)

    counter = 0

    for items in spaces_list:
        spaces_list[counter].Draw(screen)
        counter += 1

    pygame.display.flip()

    clock.tick(60)

pygame.quit()
Improve this question
\$\endgroup\$
6
  • 2
    \$\begingroup\$ Did you ever think about how you could calculate the game coordinates (0..6, 0..5) from the screen coordinates (0..319, 0..239)? That can be done a lot easier than having this huge if then elif cascade. Try to explain how this works, either to a human, or to a rubber duck. You could also have a look at other solutions to the same task. \$\endgroup\$ – Roland Illig Dec 18 '19 at 20:31
  • \$\begingroup\$ Instead of using a long if/else block, consider using a dictionary. \$\endgroup\$ – user182606 Dec 19 '19 at 7:57
  • \$\begingroup\$ how can I use a dictionary to fix it? \$\endgroup\$ – Moleman Dec 19 '19 at 19:21
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Ethan Bierlein Dec 19 '19 at 20:02
  • \$\begingroup\$ oops, sorry about that. Should I post another question with the revised code? \$\endgroup\$ – Moleman Dec 19 '19 at 20:08
9
\$\begingroup\$

Space

class Space():

    def __init__(self):
        self.color = WHITE
        self.height = 40
        self.width = 40
        self.x = 5
        self.y = 5
    def Draw(self, screen):
        pygame.draw.ellipse(screen, self.color, [self.x, self.y, 40, 40])

According to PEP-8, methods should use snake_case. MixedCase is reserved for class names. So Draw should be draw. Also, there should be a blank line before def draw(...).

The members height and width are never used. The members x and y are only used inside of the Space class (although they are changed externally shortly after creating the Space object. Perhaps these should be passed in as arguments to the constructor. Also, these should be named with a leading underscore to convey they are non-public members.

Perhaps better, instead of constructing the list [self._x, self._y, self._width, self._height] every time you draw the space object, you could store a list with this information:

class Space():

    def __init__(self, x, y):
        self.color = WHITE
        self._rect = [x, y, 40, 40]

    def draw(self, screen):
        pygame.draw.ellipse(screen, self.color, self._rect)

Spaces

Utilizing the new constructor:

x_list = [5, 50, 95, 140, 185, 230, 275]
y_list = [5, 50, 95, 140, 185, 230]

spaces_list = []
x_counter = 0
y_counter = 0

for i in range(42):
    slot = Space(x_list[x_counter], y_list[y_counter]

    spaces_list.append(slot)

    x_counter += 1
    if x_counter == 7:
        y_counter += 1
        x_counter = 0

This construction technique is very verbose. More over, you've got magic numbers scattered around (42, 7), which makes the code hard to understand. Where did these numbers come from? What do they mean?

You could help by declaring named constants COLUMNS = 7 and TOTAL_SPACES = 42, and then using these symbol names, but that is missing the point. You've got an x_list with 7 entries, so COLUMNS = len(x_list) makes it so that if you add an extra column to x_list, the COLUMNS value will be automatically computed. Same for TOTAL_SPACES, it would be the product of the lengths of x_list and y_list.

But storing each Space object in a 42 element 1-dimensional list is just plain wrong. You've got a 2-d grid of spaces. You should store it as a 2-d structure. A list of lists:

spaces_grid = [ [...], [...], [...], [...], [...], [...], [...] ]

You could then access the spaces like spaces_grid[x][y], with \$0 \le x \le 6\$ and \$0 \le y \le 5\$.

The list comprehension expression [Space(x, y) for y in y_list] will create a list representing one column of that grid, using a particular fixed value of x, taking each y value from successive elements of y_list, and calling the Space(x, y) constructor for those values. We then need to then repeat that for each value of x in x_list:

spaces_grid = [ [Space(x, y) for y in y_list] for x in x_list]

And done in one line.

Click on Column

        (posx,posy) = pygame.mouse.get_pos()
        if posx > 5 and posx < 45:
            ... 42 lines omitted ...
        if posx > 50 and posx < 90:
            ... 42 lines omitted ...
        if posx > 95 and posx < 135:
            ... 42 lines omitted ...
        if posx > 140 and posx < 180:
            ... 42 lines omitted ...
        if posx > 185 and posx < 225:
            ... 42 lines omitted ...
        if posx > 230 and posx < 270:
            ... 42 lines omitted ...
        if posx > 275:
            ... 42 lines omitted ...

Magic numbers galore! How much pain would it be to change the disc size from 40 pixels to 55 pixels?

When the user click the mouse button, you get an (posx, posy) coordinate. You need to convert that posx value to a column index. The bisect module can help. It will search in a list of value for the position to insert another value at. We're not inserting here, but using it would allow us to quickly determine the column based on the posx and the x_list.

column_num = bisect.bisect(x_list, posx) - 1

You could then validate that you haven't clicked between columns:

if column_num >= 0 and posx < x_list[column_num] + 40:
    column = spaces_grid[column_num]

Apologies for the magic number 40. Turn this into a named constant too.

Now you can work with column and determine how far down the column the coloured disc should fall.

            if spaces_list[35].color == WHITE:
                if turn == 0:
                    spaces_list[35].color = RED
                    turn = 1
                elif turn == 1:
                    spaces_list[35].color = YELLOW
                    turn = 0
            elif spaces_list[28].color == WHITE:
                if turn == 0:
                    spaces_list[28].color = RED
                    turn = 1
                elif turn == 1:
                    spaces_list[28].color = YELLOW
                    turn = 0
            elif spaces_list[21].color == WHITE:
                if turn == 0:
                    spaces_list[21].color = RED
                    turn = 1
                elif turn == 1:
                    spaces_list[21].color = YELLOW
                    turn = 0
            elif spaces_list[14].color == WHITE:
                if turn == 0:
                    spaces_list[14].color = RED
                    turn = 1
                elif turn == 1:
                    spaces_list[14].color = YELLOW
                    turn = 0
            elif spaces_list[7].color == WHITE:
                if turn == 0:
                    spaces_list[7].color = RED
                    turn = 1
                elif turn == 1:
                    spaces_list[7].color = YELLOW
                    turn = 0
            elif spaces_list[0].color == WHITE:
                if turn == 0:
                    spaces_list[0].color = RED
                    turn = 1
                elif turn == 1:
                    spaces_list[0].color = YELLOW
                    turn = 0

Yeouch! This is just one copy of the 7 blocks of "... 42 lines omitted ..." above.

With the column list computed above, you could change this code to read:

    if column[5].color == WHITE:
        if turn == 0:
            column[5].color = RED
            turn = 1
        elif turn == 1:
            column[5].color = YELLOW
            turn = 0
    elif column[4].color == WHITE:
        if turn == 0:
            column[4].color = RED
            turn = 1
        elif turn == 1:
            column[4].color = YELLOW
            turn = 0
    elif column[3].color == WHITE:
        if turn == 0:
            column[3].color = RED
            turn = 1
        elif turn == 1:
            column[3].color = YELLOW
            turn = 0
    elif column[2].color == WHITE:
        if turn == 0:
            column[2].color = RED
            turn = 1
        elif turn == 1:
            column[2].color = YELLOW
            turn = 0
    elif column[1].color == WHITE:
        if turn == 0:
            column[1].color = RED
            turn = 1
        elif turn == 1:
            column[1].color = YELLOW
            turn = 0
    elif column[0].color == WHITE:
        if turn == 0:
            column[0].color = RED
            turn = 1
        elif turn == 1:
            column[0].color = YELLOW
            turn = 0

And delete the other 6 copies of the block. This is a huge improvement, but it is clear we can do better. We are doing exactly the same operation on the Space objects starting at column[5] and going down to column[0]. This screams loop!

    for space in column[::-1]:
        if space.color == WHITE:
            space.color = RED if turn == 0 else YELLOW
            turn = 1 - turn
            break

Breaking this down:

  • column[::-1] reverses the list of Space objects, so column[5] is returned first, and column[0] is returned last
  • When we find a space with space.color == WHITE, we do some stuff, and then break out of the for loop.
  • A ternary ... if ... else ... is used to assign RED or YELLOW to the space, based on whether turn == 0 is true or not.
  • turn = 1 - turn will flip turn back and forth between 1 and 0.

42 lines reduced down to 6.

Draw the grid

What the code used to look like:

    counter = 0

    for items in spaces_list:
        spaces_list[counter].Draw(screen)
        counter += 1

This was odd, since you were looping over all of the items in spaces_list, and then ignoring the Space that was stored in items on each iteration. Instead, you added a counter, which you manually incremented, and then looked up the element at spaces_list[counter], which would be the same items that was already stored in items. You could have simply called items.Draw(screen).

We no longer have spaces_list. Instead, we have spaces_grid, which is a 2d grid of Space objects. We'll draw this by looping over the columns in the grid, and then loop over the space in each column, and draw that space directly.

    for column in spaces_grid:
        for space in column:
            space.draw(screen)

Miscellaneous

  • You import random but never use it.
  • You should separate imports from code with a blank line.
  • You shouldn't split the mainline code up with some code before classes are declared, and some code after it.
  • I'm not sure what module colors is. I don't have it, and needed to get the color constants a different way.

Reworked Code

Here is my reworked code. It is a wee bit shorter than your original implementation: 

import pygame, bisect

class Space():

    def __init__(self, x, y):
        self.color = WHITE
        self._rect = [x, y, 40, 40]

    def draw(self, screen):
        pygame.draw.ellipse(screen, self.color, self._rect)

pygame.init()

YELLOW = pygame.color.THECOLORS['yellow']
WHITE = pygame.color.THECOLORS['white']
BLUE = pygame.color.THECOLORS['blue']
RED = pygame.color.THECOLORS['red']

size = (320, 280)
screen = pygame.display.set_mode(size)
pygame.display.set_caption("Connect 4 but Worse")
done = False
clock = pygame.time.Clock()

turn = 0

x_list = [5, 50, 95, 140, 185, 230, 275]
y_list = [5, 50, 95, 140, 185, 230]

spaces_grid = [ [Space(x, y) for y in y_list] for x in x_list]

while not done:

    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            done = True
        if event.type == pygame.MOUSEBUTTONDOWN:
            (posx,posy) = pygame.mouse.get_pos()
            column_num = bisect.bisect(x_list, posx) - 1

            if column_num >= 0 and posx < x_list[column_num] + 40:
                column = spaces_grid[column_num]

                for space in column[::-1]:
                    if space.color == WHITE:
                        space.color = RED if turn == 0 else YELLOW
                        turn = 1 - turn
                        break

    screen.fill(BLUE)

    for column in spaces_grid:
        for space in column:
            space.draw(screen)

    pygame.display.flip()

    clock.tick(60)

pygame.quit()

There is still much work that should be done on it. For instance, I don't think the x_list and y_list is the best way to determine positions. I would rework things along these lines:

DISC_SIZE = 40
GAP = 5
COLUMNS = 7
ROWS = 6

size = (GAP + (DISC_SIZE + GAP) * COLUMNS, GAP + (DISC_SIZE + GAP) * ROWS)

And then the (x, y) coordinates for each Space could be computed from a row and column number. Bigger/smaller discs? No problem! Larger gap? No problem! Different grid size? No problem! Just adjust these 4 numbers and your game grid is completely resized.

Winning

You eventually will need a 4-in-a-row check. Let me get you started. If player holds the current player's colour, then:

a_horizontal_win = all(spaces_grid[x+i][y] == player for i in range(4))

will test if the spaces starting at (x,y), passing through (x+1,y), (x+2,y), and ending at (x+3,y) all contain the that player's colour. You just need to move the starting (x,y) location around to all the legal starting points, and do something similar for vertical, and the two diagonals.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ I see, thank you. Those numbers referred to each specific space on the game board, although I'm aware that's not a great way of doing things \$\endgroup\$ – Moleman Dec 19 '19 at 0:02
  • \$\begingroup\$ @Moleman I've continued the review, and addressed the code duplication in the if...elif... tree. You should find it an improvement even over Carcigenicate's solution, which I see you tried to implement in your question post edit. \$\endgroup\$ – AJNeufeld Dec 20 '19 at 1:08
6
\$\begingroup\$

I wish I could make a more comprehensive suggestion regarding the gigantic mass under while not done:. The sheer amount of duplication there is unsustainable. I'm pretty tired though, so I'll have to stick to simpler issues.

In that chunk, you do something along these lines, many times:

if turn == 0:
    spaces_list[#].color = RED
    turn = 1

 elif turn == 1:
    spaces_list[#].color = YELLOW
    turn = 0

At the very least, I'd try to work on that. Something like creating a toggle_state function:

def toggle_function(index: int) -> None:
    global turn

    if turn == 0:
        spaces_list[index].color = RED
        turn = 1

    elif turn == 1:
        spaces_list[index].color = YELLOW
        turn = 0

Now, you can do

if spaces_list[35].color == WHITE:
    toggle_state(35)

elif spaces_list[28].color == WHITE:
    toggle_state(28)

Instead of

if spaces_list[35].color == WHITE:
    if turn == 0:
        spaces_list[35].color = RED
        turn = 1
    elif turn == 1:
        spaces_list[35].color = YELLOW
        turn = 0
elif spaces_list[28].color == WHITE:
    if turn == 0:
        spaces_list[28].color = RED
        turn = 1
    elif turn == 1:
        spaces_list[28].color = YELLOW
        turn = 0

You could even really push your luck with readability (although it will still be better), and have toggle_state return whether or not it made a change:

def toggle_state(index: int) -> bool:
    global turn

    if spaces_list[index].color == WHITE:
        if turn == 0:
            spaces_list[index].color = RED
            turn = 1

        elif turn == 1:
            spaces_list[index].color = YELLOW
            turn = 0

        return True

    else:
        return False

Now, you can do:

toggle_state(35) or toggle_state(28) or . . .

or, something like

any(toggle_state(i) for i in [35, 28, . . .]

Things to note though:

  • This is still bad. In the first suggestion, you're still needing to refer to the index number in both the == WHITE check, and when passing the number to the toggle_state. This kind of duplication can leads to errors, although it's still better than your current solution.

  • The second suggestion introduces weird behavior conceptually. Why is toggle_state returning whether or not it did anything? Because I couldn't think of a cleaner way to do it. It doesn't make a ton of sense, but it still allows for much cleaner code.

  • Both rely on global turn, and this immediately suggests that the code stinks (it does). Ideally, doing something like returning a state including turn and spaces_list would be more proper, but I think that detracts from the main point here: reducing duplication.

Beyond that though, you have too many magic numbers floating around with no clear purpose. Ideally, you should aim for that chunk to be simplified using a loop or similar construct. As long as you have everything in a rigid, hardcoded style though, that will be difficult.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ very helpful, thank you. that cut down quite a bit of my code. I'll try to work on some type of function to replace the if/else block \$\endgroup\$ – Moleman Dec 19 '19 at 19:34
  • \$\begingroup\$ @Moleman No problem. I'm not sure how practiced you are with handling duplication, but the general "formula" is to take two pieces of duplicated code like I did above, and look at what's the same and what's different. Make the identical parts the body of a function, and make the different parts parameters of the function. That alone will cover most cases when trying to reduce duplication. \$\endgroup\$ – Carcigenicate Dec 19 '19 at 22:38

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.