0
\$\begingroup\$

I would like some advice on how I could shorten my function from my battleships game (below). Essentially what it does is it checks if a coordinate is a hit and if it is, then if checks adjacent coords and shoots the one that would be a hit (it cheats, basically, which is fine).

I would like to know what I could do to make it more effective/shorter/more neat.

(If you improve it with techniques that I am not currently using, then please explain said technique. I want to use something that I can understand.)

def ai_shoots(y_coord, x_coord, all_buttons, player_1_board, ai_score):
    # print("yes")
    if ai_score == 20:
        popupwindow("The computer has won.")
    if player_1_board[y_coord][x_coord] == ': ':
        ai_score += 1
        player_1_board[y_coord][x_coord] = 'X '
        all_buttons[y_coord - 1][x_coord - 1].configure(text="X", fg="black", bg="red3")
        if player_1_board[y_coord - 1][x_coord] == ': ':
            ai_shoots(y_coord - 1, x_coord, all_buttons, player_1_board, ai_score)
        elif player_1_board[y_coord + 1][x_coord] == ': ':
            ai_shoots(y_coord + 1, x_coord, all_buttons, player_1_board, ai_score)
        elif player_1_board[y_coord][x_coord - 1] == ': ':
            ai_shoots(y_coord, x_coord - 1, all_buttons, player_1_board, ai_score)
        elif player_1_board[y_coord][x_coord + 1] == ': ':
            ai_shoots(y_coord, x_coord + 1, all_buttons, player_1_board, ai_score)
        else:
            x = random.randint(1, 10)
            y = random.randint(1, 10)
            ai_shoots(y, x, all_buttons, player_1_board, ai_score)
    elif player_1_board[y_coord][x_coord] == 'X ' or player_1_board[y_coord][x_coord] == 'O ':
        x = random.randint(1, 10)
        y = random.randint(1, 10)
        ai_shoots(y, x, all_buttons, player_1_board, ai_score)
    else:
        player_1_board[y_coord][x_coord] = 'O '
        all_buttons[y_coord - 1][x_coord - 1].configure(text="O", fg="white")
\$\endgroup\$
1
\$\begingroup\$

logic/presentation

You are mixing presentation and business logic. Both when signalling the AI won, as with the board. Do you want to look through your whole code if you decide that "_" s a better representation for clear sea than "O", or you want it in another colour than white?

Tile

As far as I remember correctly, a tile on a battleships board has a few states: (clear sea, vessel, shot but no hit, hit vessel), where clear sea and vessel have the same representation to the opponent. So instead of using a string to represent this state, better would be to use an enum

from enum import Enum

class Tile(Enum):
    CLEAR = "CLEAR"
    VESSEL = "VESSEL"
    SHOT = "SHOT"
    HIT = "HIT"

Board

I assume your board at this moment is just a list of lists of strings. It is worth it to make a slightly smarter board

class Board:
    def __init__(self, width=10, height=10):
        self.width = width
        self.height = height
        self._board = [[Tile.CLEAR] * width for _ in range(height)]

is used to create an empty board

By overloading __getitem__ and __setitem__, you can make accessing the board a lot more intuitive:

def _exists(self, x, y):
    return 0 < x <= self.height and 0 < y <= self.width

def __getitem__(self, coord):
    x, y = coord
    if not self._exists(x, y):
        raise IndexError
    return self._board[x-1][y-1]

def __setitem__(self, coord, state):
    x, y = coord
    if not self._exists(x, y):
        raise IndexError
    self._board[x-1][y-1] = state

now you can do stuff like

b = Board()
b[2, 3]
Tile.Clear
b[2, 3] = Tile.Vessel
b[2, 3]
Tile.Vessel

This way, you don't bother the user directly with the 0-indexed list of lists in a 1-indexed environment. You can even work with row names A to J if you want with only a slight adaptation to those 2 methods.

Then you can add a ship like this:

def add_ship(self, length, x, y, orientation='horizontal'):
    directions = {
        'horizontal': (1, 0),
        'vertical': (0, 1),
    }
    dx, dy = directions[orientation]
    coordinates = [
        (x + i * dx, y + i *dy)
        for i in range(length)
    ]
    try:
        if any(self[x, y] != Tile.CLEAR for x, y in coordinates):
            raise ValueError("No room for this ship")
    except IndexError: # over the edge
        raise ValueError("No room for this ship")
    for x, y in coordinates:
        self._board[x, y] = Tile.VESSEL

In your implementation of the AI, after a hit, you have the AI check all the neighbours. This can be simplified by implementing a neighbours function on the Board

def already_shot(self, x, y):
    return self[x, y] in {Tile.SHOT, Tile.HIT}

def neighbours(self, x, y):
    coordinates = ((x-1, y), (x+1, y), (x, y-1), (x, y+1))
    for x, y in coordinates:
        try:
            yield (x, y), self.already_shot(x, y)
        except IndexError:
            pass

Same goes for the actual shooting itself:

def shoot(self, x, y):
    if self.already_shot(x, y):
        raise ValueError("Already targeted this square")

    if self[x, y] == Tile.VESSEL:
        self[x, y] = Tile.HIT
        return True
    self[x, y] = Tile.SHOT
    return False 

You will need to implement a method to check whether all ships are sunk to determine whether someone has won. This method belongs to the Board, instead of on the AI method

Testing

using a class like this to keep track of the state of the board allows you to test parts of your program piece-meal. You can write a test suite so test the placing of the ships, the shooting, etc.

Turns

Where do turns show up in this? As far as I understand this, the AI keeps shooting as long as it can hit. I would expect the AI to be implemented as a class, which remembers what it has already shot etcetera, and which gets queried for a coordinate to aim at by the Game

Random shooting

If the minimum length of a ship is 2, you only need to target half of the field in while randomly looking for a ship, so only those where (x + y) % 2 is either 1 or 0, depending on where you start

AI class

This is a first shot at it. The _last_hit can use some improvement, and using a collections.deque or even a set here seems more logical than using a single value. But it shows how different methods can be used for defined beahviour

class AI:
    def __init__(self, opponent_board):
        self._board = opponent_board
        self._last_hit = None

    def random_shot(self):
        while True:
            x = random.randint(1, self._board.height)
            y = random.randint(1, self._board.width)
            if (x + y) % 2 == 0:
                continue
            try:
                hit = self._board.shoot(x, y)
                self.mark_hit(x, y, hit)
                return hit
            except ValueError:
                continue

    def mark_hit(self, x, y, hit):
        if hit:
            self._last_hit = x, y
        else:
            self._last_hit = None

    def next_turn(self):
        if self._last_hit is None:
            return self.random_shot()
        x, y = self._last_hit
        if self._board[x, y] == Tile.HIT:
            for x_, y_ in self._board.neighbours(x, y):
                try:
                    hit = self._board.shoot(x, y)
                    self.mark_hit(x, y, hit)
                    return hit
                except ValueError:
                    pass
            self._last_hit = None

        # No adjacent tiles to aim at if this got through here
        return self.random_shot()
\$\endgroup\$
  • \$\begingroup\$ Thank you. I will try to implement this to the best of my ability. I am still new to classes. \$\endgroup\$ – Simon Sep 17 '18 at 17:56
  • \$\begingroup\$ How do I start the game? And if I'm using tkinter to create two grids of buttons, one for each player, how do I translate the click of a button to the coordinate that I want to shoot at? How do I tie that into the code you have given here? \$\endgroup\$ – Simon Sep 29 '18 at 16:15
  • \$\begingroup\$ I would like all ships to be placed on the board randomly (not by the player). My ships are: [1 ship x 4 units], [2 ships x 3 units], [3 ships x 2 units], [4 ships x 1 unit]. The ships cannot be next to each other, i.e. there must be at least one empty square/unit between them (including diagonaly). \$\endgroup\$ – Simon Sep 29 '18 at 16:48
  • \$\begingroup\$ @Simon, starting the game and pushing the buttons should be covered in the GUI and the Game parts of the program, not on the board itself. You will have a Game controller which passes the turn to each of the players, and the GUI, which takes care of the presentation, \$\endgroup\$ – Maarten Fabré Oct 1 '18 at 8:52
1
\$\begingroup\$

Just here simplify Maarten Fabré's advice

With Lots duplicate code, You can simplify this

    if player_1_board[y_coord - 1][x_coord] == ': ':
        ai_shoots(y_coord - 1, x_coord, all_buttons, player_1_board, ai_score)
    elif player_1_board[y_coord + 1][x_coord] == ': ':
        ai_shoots(y_coord + 1, x_coord, all_buttons, player_1_board, ai_score)
    elif player_1_board[y_coord][x_coord - 1] == ': ':
        ai_shoots(y_coord, x_coord - 1, all_buttons, player_1_board, ai_score)
    elif player_1_board[y_coord][x_coord + 1] == ': ':
        ai_shoots(y_coord, x_coord + 1, all_buttons, player_1_board, ai_score)
    else:
        x = random.randint(1, 10)
        y = random.randint(1, 10)
        ai_shoots(y, x, all_buttons, player_1_board, ai_score)

to

    directions = ((0,-1), (0,1), (-1,0), (1,0))
    for i, j in directions:
        if player_1_board[y_coord + j][x_coord + i] == ': ':
            x, y = x_coord + i, y_coord + j
            break
    else:
        x = random.randint(1, 10)
        y = random.randint(1, 10)
    ai_shoots(y, x, all_buttons, player_1_board, ai_score)

this is same as Maarten Fabré's

def neighbours(self, x, y):
    coordinates = ((x-1, y), (x+1, y), (x, y-1), (x, y+1))
    for x, y in coordinates:
        try:
            yield (x, y), self.already_shot(x, y)
        except IndexError:
            pass

And of course you need check the bounds of player_1_board

advice skill like Maarten Fabré mentioned

def __getitem__(self, coord):
    x, y = coord
    if not self._exists(x, y):
        raise IndexError
    return self._board[x-1][y-1]

def __setitem__(self, coord, state):
    x, y = coord
    if not self._exists(x, y):
        raise IndexError
    self._board[x-1][y-1] = state

or you can just use

0 <= y_coord + j <= 10 and 0 <= x_coord + i <= 10

guess be 10 here because you used x = random.randint(1, 10)


Recursive is not needed

Another thing I concern about is recursive you used, I think it is not needed here and cost lots memory

I did a small test, and first here is my code without recursive

def ai_shoots(y_coord, x_coord, player_1_board, ai_score):
    if player_1_board[y_coord][x_coord] == ': ':
        player_1_board[y_coord][x_coord] = 'X '
        directions = ((0,-1), (0,1), (-1,0), (1,0))
        for i, j in directions:
            if 0 <= y_coord + j <= 10 and 0 <= x_coord + i <= 10 and player_1_board[y_coord + j][x_coord + i] == ': ':
                x, y = x_coord + i, y_coord + j
                break
        else:
            x = random.randint(1, 10)
            y = random.randint(1, 10)
        return x, y, ai_score+1
    elif player_1_board[y_coord][x_coord] == 'X ' or player_1_board[y_coord][x_coord] == 'O ':
        x = random.randint(1, 10)
        y = random.randint(1, 10)
        return x, y, ai_score
    else:
        player_1_board[y_coord][x_coord] = 'O '
        return x_coord, y_coord, ai_score

def start_game(board):
    x, y, score = 0, 0, 0
    while score < 20:
        x, y, score = ai_shoots(y, x, board, score)
    print("The computer has won.")

and I used tracemalloc to test the memory cost, with a really huge board

def malloc_test():
    board = [[random.choice([": ","X ", "O "]) for _ in range(1000)] for _ in range(1000)]  
    tracemalloc.start(3)
    a_b = [b[:] for b in board]
    time1 = tracemalloc.take_snapshot()
    start_game(a_b)
    time2 = tracemalloc.take_snapshot()
    stats = time2.compare_to(time1, 'lineno')
    for stat in stats:
        print(stat)
    print()

    tracemalloc.start(3)
    a_b = [b[:] for b in board]
    time1 = tracemalloc.take_snapshot()
    ai_shoots(0,0,a_b,0)
    time2 = tracemalloc.take_snapshot()
    stats = time2.compare_to(time1, 'lineno')
    for stat in stats:
        print(stat)
    print()

Here is the result of non-recursive

test.py:55: size=496 B (+496 B), count=1 (+1), average=496 B
test.py:76: size=464 B (+464 B), count=1 (+1), average=464 B
test.py:45: size=456 B (+456 B), count=1 (+1), average=456 B

and here is the result of recursive

test.py:23: size=10080 B (+10080 B), count=20 (+20), average=504 B
test.py:27: size=3528 B (+3528 B), count=7 (+7), average=504 B
test.py:86: size=504 B (+504 B), count=1 (+1), average=504 B
\$\endgroup\$
  • \$\begingroup\$ I commented the same in Maarten's post, but I comment here as well in hopes that one of you answers. How do I start the game? And if I'm using tkinter to create two grids of buttons, one for each player, how do I translate the click of a button to the coordinate that I want to shoot at? How do I tie that into the code you have given here? \$\endgroup\$ – Simon Sep 29 '18 at 16:15
  • \$\begingroup\$ this might what you need stackoverflow.com/questions/22925599/… \$\endgroup\$ – Aries_is_there Sep 30 '18 at 0:28
  • \$\begingroup\$ No, not really. Btw, what does getitem, and setitem do? \$\endgroup\$ – Simon Sep 30 '18 at 15:31

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.