You want comments on your code?
Ok :)
Below, I'll give a detailed explanation of things I noticed, and alternatives you may want to consider.
For people not used to code review, it may look like I hate your code.
This is not the case however, I think it is a useful piece of code, and most importantly, it works :)
So please consider my comments as positive remarks for possible improvement on good code.
I added '>' in front of your code lines (and nothing to my code lines) for ease of distinguishing.
Code:
> public class Populate {
This being Java, I can see you may want to encapsulate the population algorithm
in a class. However, why do you make it a full object?
Populate sounds like an action to me, not an object. Perhaps you could make all
methods static instead?
Alternatively, you could also add the Populate code to the Grid class.
Code:
> //loop for the three different sizes of ships
> for(int i=2; i<=4; i++) {
> do { //a loop that check if there is enough empty grid space before inserting into the grid itself
> direction = random(4) + 1;
> x = random(7);
> y = random(7);
Ah, magic numbers 4, 7, and 7 (and elsewhere, grid[x][y] == 0 or 1)
As a rule of the thumb, it pays to make constants or enums for each number, and use those.
That way, you document what numbers mean, and it is much easier to change sizes etc.
Code:
> gridIsEmpty = checkAll(x, y, direction, i);
>
> if(gridIsEmpty) {
> insertIntoGrid(x, y, direction, i);
> break;
> }
> } while (!gridIsEmpty);
What's the point of this while condition? You break out of the loop if it worked.
A better way to express looping is by for(;;) { ... } or by while(true) { ... }
Code:
> public boolean checkAll(int x, int y, int direction, int shipLength) {
> boolean z = false;
> if(check(x, y)) {
> for(int count = 1; count <= shipLength; count++) {
> if(!checkDirection(x, y, direction, count)) {
> z = false;
> break;
> }
> z = true;
> }
> }
> return z;
> }
The 'z = true' is done once for every iteration. Since one time is enough, I'd
suggest you initialize 'z' to true instead, and drop the assignment.
You are aware that your ship does not start at (x,y) but just next to it?
This makes the check(x, y) a bit weird. Why check at one end but not at the other end?
Also, by making 'count = 0', checkDirection() does the same as check() so you
may want to merge the check into the loop.
Code:
public boolean checkAll(int x, int y, int direction, int shipLength) {
for(int count = 0; count < shipLength; count++) {
if(!checkDirection(x, y, direction, count)) return false;
}
return true;
}
Finally, methods do not require a single exit, it is perfectly fine to write it
as above. However, this depends on your code style.
Code:
> public boolean checkDirection(int x, int y, int direction, int count) {
> boolean z = false;
>
> switch(direction) {
> case 1: z = (check(x+count, y));
> break;
> case 2: z = (check(x-count, y));
> break;
> case 3: z = (check(x, y+count));
> break;
> case 4: z = (check(x, y-count));
> break;
> }
You need the x-step and y-step mapping (from 'direction' to a difference in x and
y) twice, once here and once in check().
To ensure both routines use the same x/y positions, you can make a getXStep(direction)
and a getYStep(direction) method, and use those in your code.
One step further is to make a Direction object containing direction, which you
pass around, and query getXStep and getYStep from it.
Another step further is to make a Position object containing x & y, and use that
instead of two x and y integers.
All three are valid solutions, which one is best depends on how often you need
Direction and Position objects elsewhere in your code.
Code:
> //check to see if a the grid element at that position is in bounds and empty
"a the grid" :)
Good that you document your methods. I'd use standard javadoc for documenting methods and classes so you can leverage all tools that exist for it, but I am very document-minded.
Code:
> public boolean check(int x, int y) {
> boolean z = false;
> if((x >= 0 && x <= 6) && (y >= 0 && y <= 6))
> if(grid[x][y] == 0)
> z = true;
The parentheses around '(x >= 0 && x <= 6)' are not needed (also around the y tests). For me, they were
confusing as I expected a || somewhere (which is the only case where I use parentheses in conditions).
The grid-test could also be merged into it, but I can see you may want to keep it separate.
Compared to the other parts of the code, these lines miss a few curly braces.
I always use curly braces for multi-line statements, but perhaps you have a different code style?
In all, a nice piece of code.
Good luck with the GUI mission :)
Login
Register
FAQ
Search
). I'm trying to find the most efficient way of populating a multidimensional array (the grid of the board, 7x7 or int grid[7][7]) with different lengths of 'ships' (2, 3 and 4). Currently i have a separate class to handle this task. What basically needs to be done, is check if the array (or grid) position is occupied with something, then check the adjacent grid positions in a random direction with all three sizes of ships so the whole ship can fit into the randomly generated position when the class add it to grid. 
...my first game yay ^^