Conversation
simon-frankau
left a comment
simon-frankau
left a comment
There was a problem hiding this comment.
Algorithm looks basically ok, but there's a bug, and as there are no tests it doesn't seem to have been caught.
| { | ||
| int CountingIslands(int noRows, int noCols, boolean[][] landOrWater) | ||
| { | ||
| int index, indexj; |
There was a problem hiding this comment.
You've got a mixture of tabs and spaces, and it looks like your editor has tab == 4 spaces. When viewed on github, which thinks tab == 8 spaces, it looks wonky.
The best thing to do is be consistent, and always use just tabs or spaces. The Google coding style uses only spaces. You should be able to set your text editor up to always use just one or the other.
| // Count the islands | ||
| int count = 0; | ||
|
|
||
| for (index = 0; index < noRows; index++) |
There was a problem hiding this comment.
You could use 'row' or 'r' or something that makes the variable tie back to it being a row-counter. "index" is not very helpful. You could just use "i", as it's about as informative.
Short names are fine if they're obvious. Long, unhelpful names are never good. :)
|
|
||
| for (index = 0; index < noRows; index++) | ||
| for (indexj = 0; indexj < noCols; indexj++) | ||
| if (landOrWater[index][indexj] == true && !visited[index][indexj]) |
There was a problem hiding this comment.
"X == true" is just "X".
| if (landOrWater[index][indexj] == true && !visited[index][indexj]) | ||
| { | ||
| DFS(index, indexj,landOrWater, visited); | ||
| count = count++; |
There was a problem hiding this comment.
This is bad in Java, absolutely horrible in C/C++, where it's undefined behaviour. You're basically saying "Get me the value of 'count', and increment it later, and take that value and assign it to 'count'." In Java, the post-increment happens before the assignment, so 'count' gets re-assigned back to its old value. In other languages, it's left undefined.
Any of the following would be ok:
count++;
++count;
count += 1;
count = count + 1;
| // DFS for all neighbours | ||
| for (int index = 0; index < 4; index++) | ||
| if (isSafe(indexRow + rowNeighb[index], indexCol + colNeighb[index], landOrWater, visited) ) | ||
| DFS(indexRow + rowNeighb[index], indexCol + colNeighb[index],landOrWater, visited); |
There was a problem hiding this comment.
You can move the isSafe check to the top of DFS, and then expand this loop into just 4 recursive DFS calls, which are clearer than this loop and sets of arrays. Sometimes data-driven structures are better, but I don't think this case is complex enough to make it worth it.
2 participants
I created the Assignment4 folder and I tried to clean this repo in order to have all the assignments organised.