22
\$\begingroup\$

The idea is, you enter an amount of ships, for example 10. It will then place 10 ships on a 10x10 grid (0-9). You may enter a coordinate, and it will tell you if it is a hit or a miss. A simplified version of the well-known board game.

  • C indicates 'Computer'. These are not shown on the grid, but are stored in the 2D array.
  • H indicates 'Hit'. Self explanatory.
  • M indicates 'Miss'. Also self explanatory.
  • Null / whitespace indicates that nothing has been set at that coordinate.

I created this program with the intent of making it as efficient and readable as possible. It's difficult to explain the inner workings of my mind, so I apologize for no comments.

best ran outside of IDE (in console)

code:

import random, os

ships = int(input("Enter ship count: "))
gridSize = 10
game = [[' ' for x in range(gridSize)] for y in range(gridSize)]

def getGrid(x,y):
    return game[x][y]

def setGrid(x,y,char):
    game[x][y] = char

for i in range(ships):              
    x = random.randint(0,gridSize-1)
    y = random.randint(0,gridSize-1)
    setGrid(x,y,'C')

xLabel = " " * 3
for i in range(gridSize):           
    xLabel += str(i) + " "

result = "Make a move!"
hits = 0

while hits != ships:
    os.system("cls")                
    print(" " + result + " [Ships: " + str(ships) + ", Size: " + str(gridSize) + ", Hits: " + str(hits) + "]\n")
    print(xLabel)                   
    for x in range(gridSize):
        print(" " + str(x) + " ",end="")
        for y in range(gridSize):
            print(" " * 2 if getGrid(x,y) == 'C' else getGrid(x,y) + " ",end="")
        print("")

    xGuess = int(input("\n X: "))
    yGuess = int(input(" Y: "))

    if getGrid(xGuess,yGuess) == 'C':
        result = "Hit! (" + str(xGuess) + ":" + str(yGuess) + ")"
        setGrid(xGuess,yGuess,'H')
        hits += 1
    else:
        result = "Miss! (" + str(xGuess) + ":" + str(yGuess) + ")"
        setGrid(xGuess,yGuess,'M')

print("\nCongratulations, you won the game!")
os.system("pause >nul")
\$\endgroup\$
14
\$\begingroup\$

Your program is pretty good, imports at the top, some functions and comprehensions. But they can all be improved.

  • To adhere to PEP8 you should only import one module per import.
  • You need some more functions, the ones your are using I would not use and would infuriate me.
  • This is more an unwritten notion, but _ is known as the throw away variable. Since you throw away the range's items you could do:

    [' ' for _ in range(gridSize)]
    

I'd change the way you display the rows, currently you display two characters per print, this is quite hard to read, and not very efficient. Instead I'd recommend a list comprehension!

You display a space if getGrid(x,y) is "C", otherwise getGrid(x,y), for each character in that row. So:

row = [" " if getGrid(x, y) == "C" else getGrid(x, y) for y in range(gridSize)]

To then display this row you should use ' '.join(row). This puts a single space in-between each position on the board.

The other major change I'd make is the use of str.format. Take the following line:

print(" " + result + " [Ships: " + str(ships) + ", Size: " + str(gridSize) + ", Hits: " + str(hits) + "]\n")

That's hard to read with all the string concatenations and conversion, which str.format tidies up. The equivalent would be:

print(" {} [Ships: {}, Size: {}, Hits: {}]\n".format(result, ships, gridSize, hits))

You can now tell the format of the string, and can reasonably guess what will go there. All in little to no time.


For a few additional changes I'd recommend you change range with enumerate to get the x positions and the row, where you display the grid. And I'd completely remove the range when using the row comprehension. To show this:

for x, row in enumerate(game):
    print(' {} {}'.format(x, ' '.join([" " if y == "C" else y for y in row])))

I would also change the boards variable name to board so that people can instantly understand what you mean when you reference it.

In Python you should use snake_case not pascalCase for functions and variables, so gridSize should be grid_size.

And finally, you should use a try-except block when you are converting user input to a number as CAD97 put in their answer. This is as your program will crash if I enter "a".

To improve the xLabel creation you can think of it like we did the row, however you display the item you get from the range. This means that the list comprehension that you'd do is:

row = [str(i) for i in range(gridSize)]
xLabel = " " * 3 + " ".join(row)
\$\endgroup\$
  • \$\begingroup\$ I did do some research into the naming convention for Python functions. While "spaced_name" might be the norm, I just prefer "mixedName". Thanks for your reply, kind stranger. \$\endgroup\$ – user102973 Apr 15 '16 at 18:23
  • \$\begingroup\$ Using _ as throw-away will break your code in the debugger. There _ is the return value of the last statement. I would use something else, perhaps ignored. \$\endgroup\$ – Martin Ueding Apr 16 '16 at 21:00
  • \$\begingroup\$ @MartinUeding "There _ is the return value of the last statement.", In IDLE it is, not in Python scripts. _ is also used for the i18n translation lookup. The three are documented here. Also I don't know about the debugger thing, could you say which one, with a small script? \$\endgroup\$ – Peilonrayz Apr 16 '16 at 21:14
9
\$\begingroup\$

First, let me say that I love the way you've done your interface. It's very nice and neat for a command-line program. User-wise, this is a nice design.

But let's get on to the potential improvements.

First, a couple things about input.

 Make a move! [Ships: 10, Size: 10, Hits: 0]

   0 1 2 3 4 5 6 7 8 9
 0
 1
 2
 3                 M
 4
 5
 6
 7
 8
 9

 X: 3
 Y: 8

You've mixed up your X and your Y coordinates. I've specified to go to (3,8) -- 3 units over and 8 units down -- and you've gone to (8,3) -- 8 units over and 3 units down. This is just a simple matter of realizing that your 2D List is in row-major order, and you have to index it as game[y][x] instead of game[x][y].

A 2D list is nothing special; it is just a list of list, and as such it has a structure like this:

[0:[0:'a', 1:'b', 2:'c'],
 1:[0:'d', 1:'e', 2:'f'],
 2:[0:'g', 1:'h', 2:'i']]

so to access an element you have to specify the row first (y) and then the column (x). As SuperBiasedMan said, you can do this easily just by using your getGrid and setGrid.

Secondly for input, this is something that we would prefer not to see: ValueError: invalid literal for int(). As your program is currently laid out, providing invalid input, such as the empty string or a non-number, will crash the program. If at all possible (and it is), you want to handle bad input gracefully. I will cover this in a bit.

So, on to talking about the code itself. After all, this is Code Review.

In Python, though you can place code at the lowest indent level to have it run, it is best to put it in a function. To do so, you follow this general outline:

import random, os

# your functions

def main():
    # your code

if __name__ == "__main__":
    main()

This encapsulates your logic into a main function and calls it when you run this script. Check This StackOverflow question for a great explanation of if __name__ == "__main__". Basically, it tells Python to only run this code when the script is run as the main task.

Now that we're defining functions, let's take care of the input problem. The basic idea of a fail-safe input is to try to parse the input, catch any errors if they exist, then try again if we failed. So, input_int could look something like this:

def input_int(prompt):
    while True:
        try:
            return int(input(prompt))
        except ValueError:
            print('Bad input; try again.')

Then, instead of wherever you call int(input("some string")) you use input_int("some string").

Joe Wallis has great advice on how to format your strings, so I will skip over covering that.

After that the only thing left to mention are magic constants. Rather than reference 'C', 'H', and 'M' directly in your code, define names for these values. Python doesn't have an explicit constant declaration, but the standard is SHOUTY_CASE for constants.

HIT = 'H'
MISS = 'M'
COMPUTER = 'C'
NONE = ' '

Then it will be easy to change these characters should you ever want to.

And again, Pep 8 is the official Python style guide. If you want to write code others can understand, I would highly suggest giving it a read.

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

I don't think you need getGrid or setGrid (at least not in their current form). getGrid(x, y) is actually just less clear and longer than game[x][y]. People are familiar with what the indexing syntax means, but a function call could be doing anything.

On that note, why not keep getGrid so it can do something. You never check if the player has entered valid inputs. You just pass them directly. You could easily add a try except to catch index errors to make getGrid a valuable function:

def getGrid(x, y):
    try:
        return game[x][y]
    except IndexError:
        return None

This means that instead of having ugly program breaking errors you can return no result from this function.

\$\endgroup\$
  • \$\begingroup\$ Hey there, thanks for your reply. Not sure why I did that to be honest, but your advice is completely correct. I didn't make checks intentionally, as it's not a program that will be "quality controlled". \$\endgroup\$ – user102973 Apr 15 '16 at 17:12
5
\$\begingroup\$

I love little command line games like this-- reminds me of summer nights with the C64. The only thing that bugs me is that you're randomly choosing a new grid location, but you're not checking to see if it's already occupied.

Besides the logistical difficulties in parking one battleship atop another, if random.randint picks two duplicate x, y locations, you'll never achieve an end-game condition.

In my mind, this is the perfect opportunity for simple recursion! Recursion is fun, because it's slightly mindbending (at first), and never fails to impress the ladies. (Full disclosure: my wife is not impressed by recursion.)

I'd break out the 'random.randint' assignments to a separate function and do something like this:

def picker():
    x = random.randint(0,gridSize-1)
    y = random.randint(0,gridSize-1)
    if getGrid(x, y) == 'C':
        x, y = picker()
    return x, y

It checks to see if that location is in use. If so, it just calls itself, again (and again, possibly), and returns the final result. Then, change your ship placement loop to this:

for i in range(ships):              
    x, y = picker()
    setGrid(x,y,'C')

Of course, if you decide to place 101+ ships, it'll run until it kicks out a maximum recursion depth error. (which won't take long)

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! I realized myself this error not too long ago, but could not come up with an elegant solution. \$\endgroup\$ – user102973 Apr 15 '16 at 20:13
  • 3
  • 3
    \$\begingroup\$ I wouldn't say that this is a perfect opportunity for recursion. A while loop would do the job, and it would be faster, more idiomatic, and less prone to stack overflow than recursion. \$\endgroup\$ – 200_success Apr 15 '16 at 21:02
  • \$\begingroup\$ @200_success - True! But, if the choice is an exception that dumps the program and points to the problem, or a while loop that runs infinitely without fuss, it might be nicer (for debugging purposes) to get the exception. Sure, you could code a while loop that doesn't behave like that, but..... heck, I don't have a better justification. I just like recursion for this sort of solution. \$\endgroup\$ – Winky Apr 15 '16 at 21:20
2
\$\begingroup\$

Your os.system('cls') will only work on windows. Perhaps you can make it work cross platform, wouldn't that be marvellous.

import os
def cls():
    os.system('cls' if os.name=='nt' else 'clear')
cls()

Then after defining function cls, all you need to do now is one-liner cls() to clear your screen.

References: Stackoverflow question.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy