Hi, I wondered if anyone could take a look at a simplified Battleship game I've written. It's my first piece of code so I'd appreciate any advice about how it could be improved - in particular the layout and commenting. As far as I know there are no bugs but I'm likely to be wrong...

print 'Welcome to Battleship. The object of the game is to locate and destroy the enemy ship. You must correctly guess two co-ordinates occupied by the ship to destroy it. This is a 1-player game with a single enemy ship, and you have 10 turns.'print

#checks to see if player has guessed correctly: if level=='easy': if [guess_row,guess_col] in ship_e: correct_guess(guess_row,guess_col) if check_sunk(ship_e): print "Congratulations! You sunk my battleship!\n" else: incorrect_guess(guess_row,guess_col) elif level=='hard': if [guess_row,guess_col] in ship_h: correct_guess(guess_row,guess_col) if check_sunk(ship_h): print "Congratulations! You sunk my battleship!\n" else: incorrect_guess(guess_row,guess_col)

Last edited by ovisam on Tue Dec 24, 2013 6:29 pm, edited 1 time in total.

The layout could be improved by having no floating code. That is, no code at the global level (unindented) that isn't a function definition. That forces everything to be in nice, modular functions to which you would assign nice names and doc strings. When I read code I tend to ignore comments that aren't doc strings. For example, in your code that gets the difficulty choice, your comment would be better off just being a well-named function definition, perhaps with a doc string (never hurts). Same for creating the board, and then the comment for printing the board is redundant to the function name.

There are some nice idioms in Python too, which you mean by interested. For example you have this code

On line 19 where you use the string literal "O " I wondered why the space, and guessed that "O" means "open". A good habit to get into is to not use "magic literals" (people often talk about "magic numbers") which are literals that appear in code without meaningful naming and which you might see elsewhere but can't know if they mean the same thing in different places. The convention is to create a "constant" which you can and often should put in the global namespace, but it will be in ALL_CAPS and you will, by convention, never modify it (some languages have provisions to prevent entirely modifications, Python does not).

As for the space, if it's a display issue, I would simply use two spaces in the string you use with join(). And O, H, and X could all probably be renamed as global constants (OPEN, HIT, and MISS?).

On lines 87 and 91 you have logic to check for easy/hard quite late in your program. All you're doing there is looking at which of the two datastructures you created though, the important logic is the same. You can simplify

Simply pass the relevant one in as a function argument. You probably don't need to create both of them when you do anyway, just create the one you're actually going to use.

In your mainloop, you always increment turn. Just loop over the proper values by passing 1 as the first argument to range(). (By the way, a good Python 2 habit is to use xrange() instead of range() for reasons I won't go into here but you can Google.) When you need to detect win/loss, you can do so with Python's else branch of a for loop. I include an example below of what it does

Thanks for taking the time to have a look! I really appreciate your advice and suggestions. I've modified the original post to include the updated version of the programme, but I'm having a few problems (warning - it's not running right now)

As per your suggestion, I put the code for selecting the difficulty choice and creating the board into functions, then call them at the start of the game. For some reason, I get errors saying 'board' and 'level' aren't defined - I don't understand why, because before these functions I call select_difficulty_level which defines the level and I call the create_board function which defines the board.

micseydel wrote:On line 19 where you use the string literal "O " I wondered why the space, and guessed that "O" means "open". A good habit to get into is to not use "magic literals" (people often talk about "magic numbers") which are literals that appear in code without meaningful naming and which you might see elsewhere but can't know if they mean the same thing in different places. The convention is to create a "constant" which you can and often should put in the global namespace, but it will be in ALL_CAPS and you will, by convention, never modify it (some languages have provisions to prevent entirely modifications, Python does not).

As for the space, if it's a display issue, I would simply use two spaces in the string you use with join(). And O, H, and X could all probably be renamed as global constants (OPEN, HIT, and MISS?).

The reason I have the "O", "H" and "X" string literals is so that when I print the board, the player gets a visual representation of the board, something like this to keep track of hits and misses:

I appreciate that this is confusing for anyone reading the code... Would there be a better way of doing this though? Perhaps I could add a comment explaining what I'm doing.

micseydel wrote:In your mainloop, you always increment turn. Just loop over the proper values by passing 1 as the first argument to range(). (By the way, a good Python 2 habit is to use xrange() instead of range() for reasons I won't go into here but you can Google.) When you need to detect win/loss, you can do so with Python's else branch of a for loop.

I'm a bit confused about how to implement the for/else loop - in my current version of the program, it seems like the "turn" value is increasing by 1 until it hits 10, and then the 'else' part is carried out. When you say increment 'turn', do you mean write turn+=1?

We're happy to help! I know people can sometimes be self-conscious about their code, which inhibits improvement, so kudos for posting here.

By the way, it would be better if in the future you left the original version and then posted the updated code, rather than removing the old, which can be a useful reference (especially if the old code worked and the new doesn't ).

ovisam wrote:As per your suggestion, I put the code for selecting the difficulty choice and creating the board into functions, then call them at the start of the game. For some reason, I get errors saying 'board' and 'level' aren't defined - I don't understand why, because before these functions I call select_difficulty_level which defines the level and I call the create_board function which defines the board.

When you get errors, don't just provide a snippet of them, please provide the entire traceback verbatim. For example, when I run the following code in IDLE,

Including this full traceback verbatim and in code tags really helps us to figure out your problem quickly. The problem here is that "limited_scope_variable" is local to the function, so I can't access it from outside the function (here the global scope) without making it available explicitly. This is done so with function parameters and return values, which you can read about in any tutorial on functions. After you take a look at one of those if you're still having trouble feel free to ask any questions you have about that.

As for the spaces at the end of your strings, you don't need them to be in the strings you're using since when you do the formatting with join() you can solve that problem there and then. When you do

Also, in case this wasn't clear before, you can create constants to represent the values because they're named, but because the variable represents a string when you print it the user gets the intended experience.

ovisam wrote:I'm a bit confused about how to implement the for/else loop - in my current version of the program, it seems like the "turn" value is increasing by 1 until it hits 10, and then the 'else' part is carried out. When you say increment 'turn', do you mean write turn+=1?

The majority of your code is in the for part, the else branch will only contain code you wish to be executed when the loop completes without you using break. And just for some clarity here, the loop

The else branch of a for-loop gets executed when you run out of things in the list. If you use break, you don't run out, you exit the entire construct (for-else-loop) early. You usually don't want to use the loop variable in the else part by the way; it'll always be the last element (since Python leaks out the variable you name in the for loop rather than it just being in the for loop).

When I say you're incrementing turn, I mean that you're literally adding 1 to turn every time you print it out

print 'Welcome to Battleship! The object of the game is to locate and destroy the enemy ship. You must correctly guess two co-ordinates occupied by the ship to destroy it. This is a 1-player game with a single enemy ship, and you have 10 turns.\n'

#checks to see if player has guessed correctly: if [guess_row,guess_col] in ship: correct_guess(guess_row,guess_col) if check_sunk(ship): print "Congratulations! You sunk my battleship!" break else: incorrect_guess(guess_row,guess_col)

else: #if player has used up their 10 turns: print "You're out of turns! Game over."

Got a few things I'd like to check:1. I've replaced 'O' with OPEN etc and put these definitions at the start of the program - is that ok or would it be better practice to put these elsewhere?2. The game works the way I want it to but I'm a bit concerned about something regarding local and global variables. When the player, say, misses and their guess is replaced by an X on the board, that new definition is local to the incorrect_guess function, right? But when the player guesses correctly and the correct_guess function is called, at the end of the function the new board is printed with the previous misses. Why does it not ignore the misses and just print the original board plus the new hit?

Kudos on getting it working and cleaning it up! Global constants tend to be put at the top of the program, so where you have them is good.

ovisam wrote:2. The game works the way I want it to but I'm a bit concerned about something regarding local and global variables. When the player, say, misses and their guess is replaced by an X on the board, that new definition is local to the incorrect_guess function, right? But when the player guesses correctly and the correct_guess function is called, at the end of the function the new board is printed with the previous misses. Why does it not ignore the misses and just print the original board plus the new hit?

The only global variables you should have are constants (constants by convention, technically still variables). The idea is that you should have one nice main() function that is very readable at a high level, that passes around all the information that anything else needs. Soon you'll want to consider object oriented programming if you want to tidy things up further, although OOP is an advanced topic in programming so you should be quite happy with the progress you've made here.

Before giving further advice I'll leave it to you to remove all the global variables you have, since you did quite a good job at this first pass and so you'll probably do quite well at that. I do want to reiterate that you all your code other than global constant declarations should be in functions, including that print at the top of your file. When someone imports your file, there shouldn't be any output or anything.

Join the #python-forum IRC channel on irc.freenode.net for off-topic chat!

Please prefer not to PM members. The point of the forum is so that anyone can benefit. We don't want to help you over PMs/emails/Skype chats that others can't benefit from

def welcome(): print 'Welcome to Battleship! The object of the game is to locate and destroy the enemy ship. You must correctly guess two co-ordinates occupied by the ship to destroy it. This is a 1-player game with a single enemy ship, and you have 10 turns.\n'

#checks to see if player has guessed correctly: if [guess_row,guess_col] in ship: correct_guess(guess_row,guess_col,board,ship) if check_sunk(ship): print "Congratulations! You sunk my battleship!" break else: incorrect_guess(guess_row,guess_col,board)

else: #if player has used up their 10 turns: print "You're out of turns! Game over."

#checks to see if player has guessed correctly: if [guess_row,guess_col] in ship: correct_guess(guess_row,guess_col,board,ship) if check_sunk(ship): print "Congratulations! You sunk my battleship!" break else: incorrect_guess(guess_row,guess_col,board)

else: #if player has used up their 10 turns: print "You're out of turns! Game over."