Simple Battleship written in Python

Question

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
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.
 """
 hitMapArray = []
 file = open(dataFileName, "r")
 lines = file.readlines()
 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
def LoadScoreFromFile(dataFileName):
 """ 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")
 lines = file.readlines()
 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()
 if not userName:
 print(" Error! Please enter a name.")
 continue
 scoreList.append([userName, score])
 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 
def UpdateShips(hitCoords, ships, hitMapArray):
 """ 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
 scoreList = LoadScoreFromFile("scoreList.score")
 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] != " ":
 print(" You guessed that already.")
 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):
 UpdateShips(guessCoords, shipsDict, hitMapArray)
 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
 hitMapArray = LoadMapFromFile("map.map")
 # 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

• Sᴀᴍ Onᴇᴌᴀ: 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.
• Paulo Scardine: Take a look at PEP8 - it is a document about naming conventions and other stylistic matters for Python code.
• Snowbody: 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.

Answer 1

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")
 lines = file.readlines()
 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.

As for your other functions:

• 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.

• Solomon Ucko: 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.
• andow rannl: 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!
• Snowbody: 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: @andowrannl That's unfortunate -- too bad the school didn't have their teacher actually teach a language she/he knows.

Answer 2

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]

• andow rannl: 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.
• Graipher: @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.
• Snowbody: 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.
• Graipher: @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.