1

it's me again. I'm trying to make a very simple Battleships game. Now I want to make ships be separated with at least one free cell. Look at the picture

enter image description here

As you see, the edge is completely ship-free, which makes available number of placement much smaller (or less? sorry for my bad English).

The "Field" is simply an int[10][10] array.

I have a very rude method to check avaibality to place ship: (IS_WATER const = 0)

private static boolean checkIfShipAvailable(int x, int y, int dir, int length) {
         int counter = 0;
        switch(dir) {
        case DIRECTION_RIGHT:
            try {
                if(field[x-1][y] == IS_WATER) counter++;     
                if(field[x-1][y-1] == IS_WATER) counter++;    
                if(field[x-1][y+1] == IS_WATER) counter++;    
                if(field[x][y-1] == IS_WATER) counter++;      
                if(field[x][y+1] == IS_WATER) counter++;     
                if(field[x+1][y-1] == IS_WATER) counter++;    
                if(field[x+1][y+1] == IS_WATER) counter++;    

                if(field[x+length-1][y-1] == IS_WATER) counter++;
                if(field[x+length-1][y+1] == IS_WATER) counter++;
                if(field[x+length][y] == IS_WATER) counter++;
                if(field[x+length][y-1] == IS_WATER) counter++;
                if(field[x+length][y+1] == IS_WATER) counter++;

            } catch (IndexOutOfBoundsException e) {
                counter++;
            }
            Log.d(TAG, "Direction: Right. Counter = " + counter);
            if (counter == 12)
                return true;
            break;

        case DIRECTION_DOWN:
            try {
                if(field[x-1][y-1] == IS_WATER) counter++ ;
                if(field[x][y-1] == IS_WATER) counter++ ;
                if(field[x+1][y-1] == IS_WATER) counter++ ;
                if(field[x-1][y] == IS_WATER) counter++ ;
                if(field[x+1][y] == IS_WATER) counter++ ;
                if(field[x-1][y+1] == IS_WATER) counter++ ;
                if(field[x+1][y+1] == IS_WATER) counter++ ;

                if(field[x-1][y+length-1] == IS_WATER) counter++ ;
                if(field[x+1][y+length-1] == IS_WATER) counter++ ;
                if(field[x-1][y+length] == IS_WATER) counter++ ;
                if(field[x][y+length] == IS_WATER) counter++ ;
                if(field[x+1][y+length] == IS_WATER) counter++ ;

            } catch (IndexOutOfBoundsException e) {
                counter++;
            }
            Log.d(TAG, "Direction: Down. Counter = " + counter);
            if (counter  == 12)
                return true;
            break;
        }
        return false;
    }

That's for 2-4 cell ships. And for 1-cell ship:

private static boolean checkIfOneAvailable(int x, int y) {
         int counter = 0;
         try {
                if(field[x-1][y-1] == IS_WATER) counter++ ;
                if(field[x][y-1] == IS_WATER)counter++ ;
                if(field[x+1][y-1] == IS_WATER) counter++ ;
                if(field[x-1][y] == IS_WATER) counter++ ;
                if(field[x+1][y] == IS_WATER) counter++ ;
                if(field[x-1][y+1] == IS_WATER) counter++ ;
                if(field[x+1][y+1] == IS_WATER) counter++ ;
                if(field[x][y+1] == IS_WATER) counter++ ;   
        } catch (IndexOutOfBoundsException e) {     
          counter++;
        } 
         if (counter == 8) 
             return true;
        return false;

     }

Could you tell me, where's the mistake and how I can make ships be placed even on edges?

3
  • You don't want to test for water. You want to check if there's not another ship already. Jun 11, 2013 at 15:35
  • IS_WATER means empty cell
    – Groosha
    Jun 11, 2013 at 15:40
  • I see that. But if you try to do it by testing for ships, the algorithm will be easier to understand. Jun 11, 2013 at 15:47

2 Answers 2

2

Don't test for water. Test if another ship is there. Like this:

private static boolean checkIfOneAvailable(int x, int y) {        
    if(x!=0 && y!=0 && field[x-1][y-1] == IS_SHIP) return false;
    if(y!=0 && field[x][y-1] == IS_SHIP) return false;
    ...
    return true;
 }

Note that I've added the checks for array bounds to the condition. Using exception for flow control is considered a bad habit mainly because it's slow. When using this approach, you'll also get the performance gain from the fact, that you don't always have to check all the surrounding fields. Once you find a collision, you get the result.

Also there is no point in copy-pasting the code for larger ships. Until you have placed the ship, you can still do the same check for every field of the larger ships:

private static boolean checkIfAvailable(int x, int y, int dir, int length){
    for (int i = 0; i<length; i++){
        switch (dir) {
            case DIRECTION_RIGHT:
                if (!checkIfOneAvailable(x+i,y))
                    return false;
                break;
            case DIRECTION_DOWN:
                if (!checkIfOneAvailable(x,y+i))
                    return false;
                break;
        }             
    }
    return true;
}
4
  • I don't think it will be applicable for larger ships, as field[x+1][y] while Direction is right will always give false, for example. But it's just a note
    – Groosha
    Jun 11, 2013 at 16:04
  • I don't understand. why would it be false? I thought you're placing the whole ship after you check if it can be placed. So If you have a ship 2*1, then you check if both pieces of the ship can be placed. Where's the catch? Jun 11, 2013 at 16:06
  • Could you please look at this: pastebin.com/LeH6bANt ... no ships are placed on bottom and right side..
    – Groosha
    Jun 11, 2013 at 16:47
  • There may also be a problem in other methods. I think that what you've posted is fine. Try it with debugger. It's not hard at all to set it up and you'll find plenty of tutorials. Jun 11, 2013 at 17:07
1

If an IndexOutOfBoundsException occurs all later if(field... lines up to the catch are never executed and the counter isn't increased anymore.

Better solution: Create a method which takes coordinates and returns either the field[x][y] value or a special OUT_OF_BOUNDS value (by catching IndexOutOfBoundsException or checking the bounds)

4
  • you mean (if ((field[x+1][y] == IS_WATER) || myNewMethod == true) ?
    – Groosha
    Jun 11, 2013 at 15:36
  • 1
    You beat me to it! In the future he could also run the app in debug mode and step through the method. That would give a faster response than asking Stack Overflow, though that would deprive us from fighting for rep points haha.
    – Mr. Will
    Jun 11, 2013 at 15:37
  • @Kondra007 No, replace your field[x+1][y] == IS_WATER by myNewMethod(x+1,y) != IS_SHIP Jun 11, 2013 at 15:40
  • @Mr.Will, by the way, could you give a link to explanation how to use debugger in Eclipse? I'm used to debug in my brain, but it's hard sometimes.
    – Groosha
    Jun 11, 2013 at 16:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.