9
\$\begingroup\$

I made a battleship game for a programming class, where I only had about a week to get it done. Under this time crunch, and the fact that I don't know python very well, I made some of the code in very roundabout ways, and probably far more complicated than it needs to be.

"""
This is my program that lets you play one man battleship.
It has a board that displays the info to the player, henceforth know as the player board,
and an enemy board which is hidden from the player, know as the enemy board which is used to store ship locations.
There is also an info board that has the ship locations for each board on the corosponding line.

On the enemy board, a zero is used to represent an empty tile, while an X is a ship section.
On the player board, a zero is used to represent a unguessed tile, an M the misses, and an X for hits.
"""

import random

board = []
hits = 0
random.seed()
num = random.randrange(1, 5)
sunk = [0 , 0, 0, 0]


# Loads the enemy board
def enemyBoardLoader():
    global num
    file = open("board" + str(num) + ".txt")
    return file.read()

#Loads the line of the info board that corosponds to the board chosen
def boardInfoLoader():
    file = open("boardInfo.txt")
    #Reads a line and puts it on a filler varible, as that is the only way to incremeant the readline command
    for x in range(num - 1):
        filler = file.readline()
    return boardInfoInterp(file.readline())

#Turn the line input into a list of lists
def boardInfoInterp(boardInfo):
    boardInfoList = [] #Output
    shipSet = [] #Varible 1 that is used to chunk the list
    shotSet = [] #Varible 2 that is used to chunk the list
    x = 0
    #Loop that moves it into a nested list
    while x < len(boardInfo):
        #If the letter the program is on is a number, it adds it to the second chunk varible
        try:
            shotSet.append(int(boardInfo[x]))
        #If its not, it runs this code
        except ValueError:
            #If it reaches a comma, moves the first chunk varible to the first one, then clears the first chunk varible
            if boardInfo[x] == ",":
                shipSet.append(shotSet)
                shotSet = []
            #If it reaches a semi colon, moves the second chunk varible to the ouput, making a nested list, then clears the second chunk varible
            elif boardInfo[x] == ";":
                boardInfoList.append(shipSet)
                shipSet = []
        x += 1
    return(boardInfoList)

#If a shot is on a ship, it removes that shot from the boardInfo list
def boardInfoRemover(hit):
    global boardInfo
    #Makes the input be in a format that the program needs
    hit = str(hit)
    hit = list(hit)
    #Removes extra charecters from the list
    while "[" in hit:
        hit.remove("[")
    while " " in hit:
        hit.remove(" ")
    while "," in hit:
        hit.remove(",")
    while "]" in hit:
        hit.remove("]")
    x = 0
    #Makes the numbers left in the list numbers
    while x < len(hit):
        hit[x] = int(hit[x])
        x += 1
    x = 0
    #Checks to see which shot location it was, and removes it from the boardInfo list
    while x < len(boardInfo):
        y = 0
        while y < len(boardInfo[x]):
            if boardInfo[x][y] == hit:
                del boardInfo[x][y]
            y += 1
        x += 1

# General things done to the player board
def boardFunc(func, shot):
    global board
    if func == "reset":  # Checks if command given was reset
        # Sets default board state of 100 zeros
        for x in enemyBoard:
            board.append("0")
    if func == "print":  # Checks if command given was print
        # Prints 10 rows of 10 characters
        for x in range(10):
            print(" ".join(board[(x * 10):((x + 1) * 10)]))
    if func == "update":  # Checks if command given was update
        if enemyBoard[((shot[0] - 1) + (shot[1] - 1) * 10)] == "X":  # If the shot location is an X on the enemy board, mark a hit on the player board
            if board[((shot[0] - 1) + (shot[1] - 1) * 10)] != "X":
                board[((shot[0] - 1) + (shot[1] - 1) * 10)] = "X"
                boardInfoRemover(shot)
                global hits
                hits += 1
        else:
            board[((shot[0] - 1) + (shot[1] - 1) * 10)] = "M"  # If the shot location is not an X on the enemy board, mark a miss on the player board

#Takes and cleans the input to prevent errors
def inputCleaner():
    global y, x
    num = []
    repeat = True  # Secondary variable to handle loops
    # Checks to make sure input is 2 items instead of one
    while repeat:
        try:
            xy = input("Were do you want to shoot your shot? (num1 num2):")
            x, y = xy.split()  # Gives error if its only one number
            repeat = False  # If there is an error, this does not run, making the loop keep running
        # If error is given, runs this instead of any code left to run
        except ValueError:
            print("Please enter two numbers")
    true = True  # Primary variable to handle loops
    # Main loop to check if input is right
    while true:
        true = False  # *Starts chuckling*
        try:
            # Sets the variable to numbers instead of characters they came in as. If this fails, it runs the except code below
            x = int(x)
            y = int(y)
            if y > 10 or y < 1:  # Makes sure that the first number is on the board, if not asks for another number to replace it
                true = True
                y = input(
                    "Please make sure that your first number is at least 1 and no more than 10. Re-enter only your first number:")
            elif x > 10 or x < 1:  # Same as above, but with x
                true = True
                x = input(
                    "Please make sure that your second number is at least 1 and no more than 10. Re-enter only your second number:")
            else:  # If neither of the above are true, puts the numbers in the num list
                num.append(int(x))
                num.append(int(y))
        except ValueError or TypeError:  # If something other than a number is entered, it runs this code
            repeat = True
            # This code makes sure at least two numbers were entered
            while repeat:
                try:
                    xy = input("Please use only numbers. Enter you shot location again:")
                    x, y = xy.split()
                    repeat = False
                    true = True
                except ValueError:
                    print("Please enter two numbers")
    return num

#Checks if all locations on a ship have been shot
def sunkChecker():
    global boardInfo
    global sunk
    if len(boardInfo[0]) == 0 and sunk[0] == 0:
        print("You sunk the aircraft carrier.")
        sunk[0] = 1
    if len(boardInfo[1]) == 0 and sunk[1] == 0:
        print("You sunk the battleship.")
        sunk[1] = 1
    if len(boardInfo[2]) == 0 and sunk [2] == 0:
        print("You sunk the cruiser.")
        sunk[2] = 1
    if len(boardInfo[3]) == 0 and sunk[3] == 0:
        print("You sunk the submarine.")
        sunk[3] = 1


enemyBoard = enemyBoardLoader()
boardInfo = boardInfoLoader()
while True:
    boardFunc("reset", 0)
    while hits < 15:  # Each board has 15 ship spots, when all are hit it breaks out of this loop
        boardFunc("print", 0)
        boardFunc("update", inputCleaner())
        sunkChecker()
    boardFunc("print", 0)
    if input("YOU WON!! Would you like to play again? (y/n):") == "n":
        quit()

What are some parts that could be simplified? Also, these are the specific files I have for this program.

\$\endgroup\$
1

3 Answers 3

5
\$\begingroup\$

I think the place to start is the function boardFunc. Until you get comfortable with "functional programming" and dependency inversion, you should probably never write a function that takes an argument telling it what to do. We can rewrite that function as

# Set up the player board for a new game. (100 "0"s)
def boardReset():
    global board
    for x in enemyBoard:
        board.append("0")

# Print the player board. (10 rows of 10)
def boardPrint():
    global board
    for x in range(10):
        print(" ".join(board[(x * 10):((x + 1) * 10)]))

# Update the player board based on a shot
def boardUpdate(shot):
    global board
    if enemyBoard[((shot[0] - 1) + (shot[1] - 1) * 10)] == "X":  # If the shot location is an X on the enemy board, mark a hit on the player board
        if board[((shot[0] - 1) + (shot[1] - 1) * 10)] != "X":
            board[((shot[0] - 1) + (shot[1] - 1) * 10)] = "X"
            boardInfoRemover(shot)
            global hits
            hits += 1
    else:
        board[((shot[0] - 1) + (shot[1] - 1) * 10)] = "M"  # If the shot location is not an X on the enemy board, mark a miss on the player board

I'm testing your code in Jupyter Notebooks, where the while True: ... quit() pattern doesn't work right. We can make it a little more explicit, reliable, and portable by using a mutable variable to break the loop:

keepPlaying = True
while keepPlaying:
    boardReset()
    while hits < 15:  # Each board has 15 ship spots, when all are hit it breaks out of this loop
        boardPrint()
        boardUpdate(inputCleaner())
        sunkChecker()
    boardPrint("print", 0)
    keepPlaying = "y" == input("YOU WON!! Would you like to play again? (y/n):")

In fact, you seem a bit trigger-happy with the while loops in general. If you can use a for loop, that's preferable. When you want to loop over a range of numbers, you can use range, but when you're working with an existing list you're better off looping over the list itself (or a derivation of the list). Even better is to avoid a loop altogether. While you're at it, try to work with the data structures python provides!
(I was able to confirm that your boardInfoRemover function was working for hits in the tenth row or column, but I can't explain how.)

#If a shot is on a ship, it removes that shot from the boardInfo list
def boardInfoRemover(hit):
    #pprint.pprint(hit)
    global boardInfo
    #Checks to see which shot location it was, and removes it from the boardInfo list
    for i, boardRow in enumerate(boardInfo):
        for j, value in enumerate(boardRow):
            if value == hit:
                del boardInfo[i][j]

Looking at boardReset, does it actually reset the board, or does it just assume it's already empty, and then append? I was just saying that you should use fewer loops, so now's a good time to introduce list comprehensions. (The other option would be to map with a lambda, build a const function, but comprehension are a good tool.)

# Set up the player board for a new game. (100 "0"s)
def boardReset():
    global board
    board = ["0" for _ in enemyBoard]

The inputCleaner function is large, but without bringing in a new library to do input cleaning for you, there may not be a way to make it shorter. What we can do is make it's flow more clear (linear), not rely on exceptions, and return 0-indexed coordinates. In particular, we can make it recursive, avoiding a lot of the looping behavior. We loose the part where the user only has to re-enter one of the numbers if they're out of range, but that doesn't seem like a big loss.

#Takes and cleans the input to prevent errors
def inputCleaner():
    failures = []
    coordinates = None # be careful about None, but it'll do for now.
    input_list = input("Were do you want to shoot your shot? (num1 num2):")
    if 2 != len(input_list):
        failures.append("Please enter exactly two numbers.")
    if any(not coordinate.isdigit() for coordinate in list_input):
        failures.append("Please use only numbers.")
    else:
        coordinates = map(int, input_list)
        local_names = ["first", "second"]
        for i, c in enumerate(coordinates):
            if 10 < c or 1 > c:
                failures.append("Please make sure that your {} number is at least 1 and no more than 10."
                               .format(local_names[i]))
        if not failures:
            return [c - 1 for c in coordinates]
    # If we get here, failures will not be empty.
    failures.append("Enter you shot location again.")
    for f in failures:
        print(failure)
    return inputCleaner()

Again, use the tools the language gives you to simplify. This time the boardInfoInterp function, because we need to convert it to match the 0-indexing we want to use everywhere. We want to use 0-indexing so that we can do list-lookup.

#Turn the line input into a list of lists of pairs (which are lists) of 0-indexed ints
def boardInfoInterp(boardInfo):
    return [
        [
            [int(coordinate) - 1 for coordinate in pair.split()]
            for pair
            in ship.split(",")
        ]
        for ship
        in boardInfo.split(";")
    ]

Other things we should be doing:

  • Use bools
  • Store the board as a list of lists.
  • Use arguments and return values instead of global variables.

But getting this to the point where there's no global state would amount to a complete rewrite. If you want to go that route, you'll need to think about how to best represent the game state as an abstract data structure. You'd then write functions like

newGame: ()->GameState
playerMove: (GameState, PlayerMove)->GameState
display: GameState->print
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the in-depth answer. The way the boardInfoRemover works is by taking the shot input (say, [10, 10]) turns it into a str ("[10, 10]"), turns that str into a list ([ "[", "1", "0", ",", " ", "1", "0", "]"]), removes the brackets spaces and commas (["1", "0", "1", "0"]), then finally makes all the numbers into int ([1, 0, 1, 0]). Quite ridiculous but the first thing I thought of. \$\endgroup\$
    – user211881
    Commented Nov 12, 2019 at 22:16
  • \$\begingroup\$ "you should probably never write a function that takes an argument telling it what to do" Why not? It can be very useful at times, even if you ignore the existance of functional programming the rest of the time. \$\endgroup\$
    – Mast
    Commented Nov 13, 2019 at 14:05
  • \$\begingroup\$ @Mast, There's probably a lot of useful discussion about it; higher-level functions are great, and dependency injection/inversion is its own whole thing that can be implemented a lot of different ways. That said, the original boardFunc is an excellent example of both when not to and how not to write a function that takes an argument telling it what to do: There's no overlap between the execution paths, the argument in question is a "data" Type when it's never used as "data", and the arg value is hard-coded in all of the places where it's used. \$\endgroup\$ Commented Nov 13, 2019 at 22:06
9
\$\begingroup\$

Just a quick comment for now. Unfortunately, except ValueError or TypeError does not do what you think it does. Python tries to be readable English where possible, but here it doesn't quite work. The parser parses this statement as:

except (ValueError or TypeError)

where the second part follows the rules of the or operator. If the first argument is truthy, return that truthy value, if not return the truthiness of the second argument.

For integers, which are falsey for 0 and truthy otherwise, this means:

1 or 2 -> 1
0 or 2 -> 2
0 or 0 -> 0

In addition, by default all objects are truthy if they exist (unless overwritten by the class itself). Since ValueError exists, it is parsed like this:

except (ValueError or TypeError) -> except ValueError

In other words, this cannot actually catch a TypeError:

try:
    raise TypeError
except ValueError or TypeError:
    print("caught!")
# TypeError: ...

Instead, use a tuple for multiple exceptions to be caught by the same except statement:

try:
    raise TypeError
except (ValueError, TypeError):
    print("caught!")
# caught!

Be careful not to use the tuple without parenthesis, which meant something else in Python 2 and is a SyntaxError in Python 3:

try:
    raise TypeError
except ValueError, TypeError:
    print("caught!")
# Python 2: TypeError: ...
# Python 3: SyntaxError: ...

was the same as

try:
    raise TypeError
except ValueError as TypeError:
    print("caught!")
# TypeError: ...

In other words, it overwrites the variable TypeError with the specific caught ValueError, which is not the exception being raised here.

\$\endgroup\$
5
\$\begingroup\$

Style

Code style is not "mission critical" for a beginner, but can become more important when working on larger projects. Python has an "official" Style Guide for Python Code, which is widely followed in the larger Python community. It codifies a lot of recommendations on variable naming, whitespace, and the like.

As an example, the official recommendation for variable and function names is to use lower_case_with_underscores instead of camelCase.

Documentation

The style guide also has a few rules about how function documentation should be written in order to be usable by Python's built-in help(...). The convention is to have the docstring after the def-line, """quoted in triple quotes""".

def enemyBoardLoader():
    """Loads the enemy board"""
    global num
    file = open("board" + str(num) + ".txt")
    return file.read()

Random number generation

You only really need to call random.seed(...) if you actually want to get reproducible pseudo-random numbers. Then you would pass a fixed seed value. If you only want some pseudo-random numbers, you don't need to call it, because the random number generator is seeded by default.

Also, there seems to be a small bug in your code. Since what really want are numbers between 1 and 5 (the boards in your repo are numbered 1 to 5), you will have to use random.randrange(1, 6) or even better random.randint(1, 5), since otherwise board 5 will never be selected.

Global variables

You are using global variables in a lot of places, which makes it harder to see which functions modify what part of the game state, and as a consequence of that also harder to test functions individually without resetting the global state in the mean time.

Fortunately, quite some instances of global variable usage in your code can be replaced by using (additional) function arguments. E.g. instead of

def enemyBoardLoader():
    global num
    file = open("board" + str(num) + ".txt")
    return file.read()

simply do

def enemyBoardLoader(num):
    file = open("board" + str(num) + ".txt")
    return file.read()

As you can see we where able to get rid of that pesky global keyword. Sidenote: strictly speaking global is not even necessary to read from variables from a surrounding scope, but only when trying to modify them. For instance, this can be witnessed in boardInfoLoader(), where you are using num without explicitly declaring it global. But as I said, it's best to avoid it whenever possible.

The truth

Python has a bool datatype with True and False as possible values, but that should be old news for you since you already use it in some places. Prefer to use True instead of 1 and False instead of 0 in sunk and similar situations. Then instead of sunk[0] == 0, you'd write sunk[0] == False or even more "pythonic" not sunk[0] (see how this starts to sound like plain English?).

Closing files

Whenever you open(...) a file, don't forget to .close() it properly. Otherwise this could get you into trouble in the long run. Since forgetting is so easy, Python has the so called with-statement which automagically ensures that the file gets closed no matter what happens (i.e. not even an Exception or return can stop it from doing so). To get back to the previous example, now rewritten using a with:

def enemyBoardLoader(num):
    with open("board" + str(num) + ".txt") as file:
        return file.read()

Reading the boards from disk

Reading and writing data from and to disk (aka (de)serializing) can be a tedious task as you maybe witnessed first-hand when writing boardInfoInterp() and associates. Fortunately, Python can greatly simplify your life here. Maybe you have heard of the so called JSON format, very often found in web applications. Python has a module called json which allows you to (de)serialize your data from/into a standardized format. As an example, say you have a list of lists (e.g. like your board), my_data = [list(range(4)) for _ in range(3)]. Enter json. json.load/dump allows you two load or write data from and to files with relative ease:

with open("mydata.json", "w") as output_file:
    json.dump(mydata, output_file)

mydata.json now has the following content:

[[0, 1, 2, 3], [0, 1, 2, 3], [0, 1, 2, 3]]

Simply calling

with open("mydata.json", "r") as file_:
    mydata = json.load(file_)

brings your data back to Python.

Using json might be overkill for simple things like lists of lists, but it can save you a lot of headache once it gets a little more involved.


That's it for now. Maybe I have time to look at your code later again. Till then: Happy coding!

\$\endgroup\$
0

Your Answer

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