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

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.

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]