# Simple Battleship written in Python

I would appreciate feedback on my first Battleship game written in Python for my intro to programming class.

Here is the code, the only thing in here is that I use a clear screen function from one of my other modules. I have commented that out so you don't also need the other module. The other module is included in my actual game though. I should note that I have already submitted, I am now just trying to improve myself on my own time as my teacher was somewhat lacking. Also the naming conventions are what we were told we had to do.

"""This program will create a simple, single player version of the game Battleship. It loads the ships
coords from a file as well as the high score list. User can play by guessing coords for 30 turns or
until they guess all 17 coords the 5 ships are using. Game will keep track of score and if it belongs
on the high score list at end of game then user can enter aname to be remembered by and program will
write this new scoreList to file for next run.

"""
import random
#import mine

""" This function reads a file (whether .txt or otherwise) It processes each line from that file
into a workable list of strings then this list containing just that line is appended to a 2d
list for use later on. Function returns this 2d list.
"""
hitMapArray = []
file = open(dataFileName, "r")
for num in range(len(lines)):
fileStringSplit = (lines[num])
splitString = [i for i in fileStringSplit.strip().split(",")]
processedList = []
for i in splitString:
try:
processedList.append(i)

except:
input(" Something went terribly wrong! Press any key to exit. ")
exit()
hitMapArray.append(processedList)
file.close()
return hitMapArray

def GetShipCoords(hitMapArray):
""" This function takes the hitMapArray created by LoadMapFromFile and iterates throughout array
determining which coords are taken by any ship. It stores all these coords in a 2d list of
its own for use determining if user has hit or missed. Function returns this shipCoords list.
"""
shipCoords = []
for row in enumerate(hitMapArray):
for column in enumerate(hitMapArray[row[0]]):
if hitMapArray[row[0]][column[0]] != "0":
shipCoords.append([column[0]+1, row[0]+1])
return shipCoords

""" This function reads a file in csv format (whether .txt or otherwise) which starts
with their name followed by High score. It will create a 2d list with each name/score as a line.
It also turns the scores into integer for use later. Function returns this 2d list.
"""
scoreArray = []
file = open(dataFileName, "r")
for num in range(len(lines)):
fileStringSplit = (lines[num])
splitString = [i for i in fileStringSplit.strip().split(",")]
processedList = []
for i in splitString:
try:
processedList.append(int(i))
except:
if i != '':
processedList.append(i)

scoreArray.append(processedList)
file.close()
return scoreArray

def DisplayScores(scoreList):
""" This function takes the scoreList as read from file and displays it to user at start of game
After displaying, it gives user a chance to continue to game or quit now.
"""
print("\n Current High Scores")
for i in range(len(scoreList)):
print(" {:<3} {:<27} - {}".format(i+1, scoreList[i][0], scoreList[i][1]))
exitPrompt = input(" Press enter to play or type q to quit: ").upper()
if exitPrompt == "Q":
exit()

def SortScores(scoreList, score):
""" This functions takes the scoreList as read from file and the users score this game. It
checks if either the score is greater than the least score on list OR if the list is
less than ten names long. If either of these conditions are met the list is updated with
new score after function asks user to input a name to go with the high score. Then it will
re-sort the list to put this new score in its proper position and send the new list
(only up to the tenth name) to SaveScores for writing back to file.
"""
scoreLoop = True
while scoreLoop:
if score > scoreList[-1][1] or len(scoreList) < 10:
userName = input(" You Made A High Score! Enter name to remember you by!: ").upper().strip()
print(" Error! Please enter a name.")
continue
scoreList.sort(key=lambda k: (-k[1]))
SaveScores(scoreList[:10])
scoreLoop = False

def SaveScores(newScoreList):
""" This function opens the scoreList file and overwrites it with the newly sorted scores. It only
does so if a new score has been added to the list. Determined in function SortScores.
"""
file = open("scoreList.score", "w")

for i in range(len(newScoreList)):
file.write("{},{}\n".format(newScoreList[i][0], newScoreList[i][1] ))
file.close()

def CreateMap(rows=10, columns=10):
""" This function creates the blank 10 by 10 mapList used for each game. I hardcoded in the values in
case I want to make the grid larger at some point. It returns this mapList for the program to use
each game.
"""
# Creates the 2D Data mapList and returns it
return [[' '] * columns for i in range(rows)]

def DisplayMap(mapList):
""" This functiion Takes the mapList created by CreateMap and displays it to user each turn, it changes
each turn to show hits/misses and will reset each game. I'll do one with .format and one without.
"""
#Prints the labels for the mapList
columnNames = "ABCDEFGHIJ"
print("    | " + " | ".join(columnNames) + " |")
# Print and numbers each row including any hits or misses
for number, row in enumerate(mapList, 1):
print(" {0:<2} | {1} |".format(number, " | ".join(row)))

def UpdateMapList(mapList, guessRow, guessColumn, hitOrMiss):
""" This function is called on from isHit(). It will update the mapList to add an X or O to the coords
guessed and print the HIT!! or MISS message to user.
"""
mapList[guessRow-1][guessColumn-1] = hitOrMiss
if hitOrMiss == "X":
print("\n HIT!!!!!")

else:
print("\n MISS")

def InputCoordinates():
""" This function asks user for their coord guess, it takes guess in as letter between a-j followed by
number between 1-10. It error checks to ensure valid coords are given then it translates this guess
into format used for rest of program (i.e j9 becomes 10, 9). It returns this formatted coord to be used
in rest of program.
"""
errorMessage = " Invalid Coords! Please try again."
while True:
# Get coords guess from user and check if valid
try:
guessCoords = input("\n Choose your target (A1 - J10) ")

if int(guessCoords[1:]) in range(1,11):
guessCoords = [guessCoords[0].strip().upper(), int(guessCoords[1:].strip())]
else:
print(errorMessage)
continue
except:
print(errorMessage)
continue
# check if letter part of coord is valid and convert to coord system using a dictionary
columnDict = {"A":1, "B":2, "C":3, "D":4, "E":5, "F":6, "G":7, "H":8, "I":9, "J":10}
if guessCoords[0] in columnDict:
guessCoords[0] = columnDict[guessCoords[0]]
return guessCoords
else:
print(errorMessage)
continue

def isHit(mapList, guessCoords, shipCoords):
""" This function will take in the guessCoords, it will determine if those coords are in the list of
ship positions and update with either hit or miss and will then return True if hit or False if not.
This allows the program to determine whether to update ships/score or to just keep going to next turn.
"""
# Test if hit or miss and sends info to UpdateMapList accordingly.
if guessCoords in shipCoords:
UpdateMapList(mapList, guessCoords[1], guessCoords[0], "X")
return True

else:
#Updates the mapList with an "O" saying that you missed the ship
UpdateMapList(mapList, guessCoords[1], guessCoords[0], "O")
return False

""" This function takes in the hitCoords if isHit has returned true and will look at that
spot to determine which ship was hit, it then lowers ships health by 1 and checks if ship has sunk.
if ship sunk then it displays the message to user.
"""
if hitMapArray[hitCoords[1]-1][hitCoords[0]-1] in ships:
ships[hitMapArray[hitCoords[1]-1][hitCoords[0]-1]][1] -= 1
if ships[hitMapArray[hitCoords[1]-1][hitCoords[0]-1]][1] <= 0:
print(" You sank my {}!".format(ships[hitMapArray[hitCoords[1]-1][hitCoords[0]-1]][0]))

def GameInstance(mapList, turns, score, shipCoords, shipsDict, hitMapArray):
""" This function will accept the maplist, turns, score, shipCoords, ships dictionary and hitMapArray
as arguments and will run one instance of the game. It will start by loading and displaying the
scoreList as written from prior game. It runs on a loop for 30 turns or until an end game condition
has been met.
"""
# Begin game
# Load and display score list
DisplayScores(scoreList)
#mine.clr_scr()
print("""\n Let's play Battleship!
You have 30 missiles to fire to sink all five ships.""")
DisplayMap(mapList)
# Play for 30 turns or until end game condition is met
while turns != 30:
# Get coordinates guess from user
guessCoords = InputCoordinates()
# Check and see if user already tried those coordinates, if so, loop back to line above
if mapList[guessCoords[1]-1][guessCoords[0]-1] != " ":
continue
# If valid coords entered then increase turn
turns += 1
#mine.clr_scr()
# If ship is hit then update ships health and increase score
if isHit(mapList, guessCoords, shipCoords):
score += 1
# If game end has been reached CheckWinState() will return True provided the user wishes
# to play again. Allows for the game to restart with reset maps, ship health etc
if CheckWinState(score, turns, scoreList, mapList):
break

def CheckWinState(score, turns, scoreList, mapList):
""" Every turn this funtion will determine if game has been won or lost as well as tell user
how many turns(missiles) remaining. If user wins game then it will only display the win
message, if game is either not over or has reached 0 missiles this will display the map
with latest choice and if needed the GAME OVER statement as well. It returns either
boolean decided by end game function to allow game to start new loop or if game not
over it doesn't return anything.
"""
# If user wins
if score >= 17:
print("""\n YOU SANK MY ENTIRE FLEET!
You had 17 of 17 hits, which sank all the ships.
You won, congratulations!\n""")
score += 30 - turns
# Send score list and games score to sortScore(), if score is not high score nothing
# will happen then endgame
SortScores(scoreList, score)
return EndGame()
# If user hasn't won or won yet
if score < 17:
print(" You have {} missiles remaining\n".format(30 - turns))
DisplayMap(mapList)
# if turns are up
if turns >= 30:
print("""\n GAME OVER.
You had {} of 17 hits, but didn't sink all the ships.
Better luck next time.""".format(score))
# Send score list and games score to sortScore(), if score is not high score nothing
# will happen then endgame
SortScores(scoreList, score)
return EndGame()

def EndGame():
""" This function will handle the game end loop, it will quit if user wishes to stop
or return true so that the game will loop into a new instance if user wants to keep playing
"""
endLoop = True
while endLoop:
newgame = input(" Want to play again? (Y/N) ").upper()
if newgame == "Y":
endLoop = False
return True
elif newgame == "N":
quit()
else:
print(" ERROR! Please choose Y or N.")
continue

def Run():
""" This function just creates/obtains the data needed to run a game. It sends this data to the game instance
for use that game. It does this on a loop and using a try on the whole thing in case of some unplanned
error considering the file usage etc going on.
"""
runLoop = True
while runLoop:
try:
# Define some variables needed for program
# Also clear the screen every loop of game
#mine.clr_scr()
# Get the info from map file for use determining where ships are located

# Dictionary for determining each ships health and name, it will reset every time game restarts.
# Keys are the numbers used to identify each ship in the map file. Values are a list with the
# string of the ships name followed by an integer of the ships health. UpdateShips() will use
# this to lower the integer of each ship until it hits zero at which point it is sunk.
ships = {"1":["Destroyer", 2],\
"2":["Submarine", 3], "3":["Cruiser", 3], "4":["Battleship", 4], "5":["Aircraft Carrier", 5]}
# Keep track of score
score = 0
# Keep track of turns
turns = 0
# Create a 2d list filled with positions in map taken by ships
shipCoords = GetShipCoords(hitMapArray)
# Create the mapList for displaying current map to user
mapList = CreateMap()
# Run game
GameInstance(mapList, turns, score, shipCoords, ships, hitMapArray)

except:
runLoop = False

Run()


Here is the file format for the ships map called map.map:

0,0,0,2,2,2,0,0,0,0
0,0,0,0,0,0,0,0,4,0
0,0,0,0,0,0,0,0,4,0
0,0,0,0,0,0,0,0,4,0
0,0,0,1,0,0,0,0,4,0
0,0,0,1,0,0,0,0,0,0
0,0,0,0,0,0,0,0,0,0
0,0,0,0,0,0,3,3,3,0
5,5,5,5,5,0,0,0,0,0
0,0,0,0,0,0,0,0,0,0


Here is the ScoreList.score file:

DREW,30
Any name,30

• Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. – Sᴀᴍ Onᴇᴌᴀ Dec 6 '17 at 19:35
• Take a look at PEP8 - it is a document about naming conventions and other stylistic matters for Python code. – Paulo Scardine Dec 6 '17 at 20:19
• Thanks for the accept, but it's tradition in codereview to wait 24-48 hours before accepting in order to see if any other good answers come in. – Snowbody Dec 7 '17 at 5:16

My biggest criticism of your code is that it isn't "pythonic".

What I mean by that is, it does not take advantage of the language features of Python that make it easier to write code and reduce your chances of error. Said features also make the code a lot shorter and easier to read.

For instance, let's take your initial function LoadMapFromFile(). I see a few non-pythonic constructions you use.

• Don't treat Python as C. You almost never have to access list elements by numeric index, especially if you're doing the same thing to each element of a list. If you're using range() you're probably doing something wrong. Instead you should use

for fileStringSplit in lines

• Similarly, don't pull a list apart just to put it back together again -- use the list itself. (In those rare instances where you need to do a list copy, slicing is a much easier way)

splitString = fileStringSplit.strip().split(",")

• What are you expecting could go wrong with the processedList.append(i) that you feel the need to put it in a try/except block? Especially since the code is not handling/recovering from the exception. And what if the caller wants to handle the exception? Or what if this function is run someplace that a print and user input isn't appropriate? You really ought to separate the input/output of your program from the actual logic, to improve reusability. If you really want to handle any errors that occur, put the call to the function inside try/except.

Let's see the modified function so far and see if it can be improved.

def LoadMapFromFile(dataFileName):
hitMapArray = []
file = open(dataFileName, "r")
for fileStringSplit in lines:
splitString = fileStringSplit.strip().split(",")
processedList = []
for i in splitString:
processedList.append(i)
hitMapArray.append(processedList)
file.close()
return hitMapArray


Still a few more refactorings needed.

• Instead of reading the file into memory and then looping over the array in memory, create a file object and then work with it directly using the with construct. This also has the benefit of automatically closing the file. So now we have:

def LoadMapFromFile(dataFileName):
hitMapArray = []
with open(dataFileName, "r") as file:
for fileStringSplit in file:
splitString = fileStringSplit.strip().split(",")
processedList = []
for i in splitString:
processedList.append(i)
hitMapArray.append(processedList)
return hitMapArray

• All these "create-an-empty list + append" groupings are unnecessary. Lists are full-fledged objects; use them to your advantage. Don't copy unnecessarily. First take

def LoadMapFromFile(dataFileName):
hitMapArray = []
with open(dataFileName, "r") as file:
for fileStringSplit in file:
processedList = fileStringSplit.strip().split(",")
hitMapArray.append(processedList)
return hitMapArray


which then becomes:

def LoadMapFromFile(dataFileName):
hitMapArray = []
with open(dataFileName, "r") as file:
hitMapArray = [fileStringSplit.strip().split(",") for fileStringSplit in file]
return hitMapArray


One more iteration, getting rid of the last temporary :

def LoadMapFromFile(dataFileName):
with open(dataFileName, "r") as file:
return [fileStringSplit.strip().split(",") for fileStringSplit in file]


By making this routine Pythonic, I reduced it down to three lines. It performs exactly the same work as your original 17-line function.

• GetShipCoords should use list comprehensions, not nested for/for/append. No need for a temporary list accumulator variable. Also some of the array references can be replaced by the target variable(s) of the comprehension.
• LoadScoreFromFile is basically the same as LoadMapFromFile. It needs the same refactoring. Perhaps the common code should be extracted and the varying code passed in as an argument.
• DisplayScores : Again you're using the range(len()) + indexing anti-pattern. This is definitely a place to use enumerate. Don't bother addressing list members by index; that's just asking for trouble. Also you may want to look into modern Python string formatting (the f"" syntax)
• SortScores has confusing logic. It's written as a while loop, but only the first part of it can be executed multiple times. If execution proceeds past a certain point, you will definitely go out of the loop. Refactor this so the loop only contains code that can run more than once. Also, instead of using append then sort, look up Python's bisect module and the function bisect.insort(scoreList, x) or other related routines.
• CreateMap is already using the * operator once. Use it once more and you won't need to use the for/range anti-pattern.
• UpdateMapList: change the params so you can pass/read the coords together, not as two separate items.
• UpdateShips lots of duplicated code. A temp variable may be helpful.
• The code after "All these appends are unnecessary. Lists are full-fledged objects; use them to your advantage. Don't copy unnecessarily." has an indentation error: hitMapArray.append(processedList) should be indented 1 level more. – Solomon Ucko Dec 6 '17 at 23:24
• Thanks, yah my teacher said they didn't know python and knew c and javascript so I guess that's why I haven't gotten the "pythonic" ways. The try block actually I was just lazy and it is taken from a function used like in LoadScoreFromFile() where I have to try to int(). I really liked how you broke it down for me though, thanks a lot! – andow rannl Dec 7 '17 at 4:18
• Note how Graipher and I wrote our answers at the same time (his was submitted first but I didn't see it when I was writing) and came up with literally the exact same implementation of LoadMapFromFile – Snowbody Dec 7 '17 at 5:17
• @andowrannl That's unfortunate -- too bad the school didn't have their teacher actually teach a language she/he knows. – Snowbody Dec 7 '17 at 15:19

In your SaveScores function, you should use with, to ensure that the file is properly closed (even when something raises an exception). You could also use writelines with a generator expression and tuple unpacking. I would also rename the variable (well and the function, but you said that format was given).

def SaveScores(scores):
""" This function opens the scoreList file and overwrites it with the newly sorted scores. It only
does so if a new score has been added to the list. Determined in function SortScores.
"""
with open("scoreList.score", "w") as file:
file.writelines("{},{}\n".format(*score) for score in scores)


Note that the docstring is not actually accurate. The file is overwritten regardless of the file already containing the same list.

Your function LoadMapFromFile is also needlessly complicated. I don't know what you are trying to guard against in the inner loop (you should always be able to append a value to a list). In general, you should (almost) never have a bare except but guard against specific exceptions. At least use except Exception, which allows for example Ctrl+C to still abort the program.

I would write it like this, again with with:

def LoadMapFromFile(dataFileName):
""" This function reads a file (whether .txt or otherwise) It processes each line from that file
into a workable list of strings then this list containing just that line is appended to a 2d
list for use later on. Function returns this 2d list.
"""
with open(dataFileName, "r") as file:
return [line.strip().split(",") for line in file]

• Thanks, the docstring though says determined by function SortScores as in the function SortScores determines whether SaveScores is used. I.e whether the file gets written to at all. I think if I knew enough about classes then SaveScores would be a method inside the class since it only gets used if SortScores determines it's needed. – andow rannl Dec 7 '17 at 3:23
• @andowrannl Ah, yes, you are right. In that case I would say "don't put unrelated stuff in your docstrings". When the function is called is irrelevant (for the function). What it does is relevant. – Graipher Dec 7 '17 at 5:27
• The idea is that all code you write should be loosely coupled to the caller, so that you can reuse the code somewhere else as-is without having to modify it. – Snowbody Dec 7 '17 at 14:30
• @Snowbody I think you meant to address the OP with that comment. If that is the case, you should ping them, they will not see it otherwise (since it is a comment on an answer and not on the question). And I agree, that is the underlying reason. – Graipher Dec 7 '17 at 14:31