Conversation
karineek
requested review from
0xlbr,
AimeeBorda,
NataliaDymnikova,
alice006,
simon-frankau and
veronikakolejak
NataliaDymnikova
left a comment
NataliaDymnikova
left a comment
There was a problem hiding this comment.
I like your code :)
But I think it coulde be shorter.
| return; | ||
| } | ||
|
|
||
| if (map[curr_row][curr_col] == false) |
There was a problem hiding this comment.
I think you could write just !map[curr_row][curr_col] without == false
| if (m_visisted_map[curr_row][curr_col] == true) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Also I think that you could combine all if here to the one or split it to the two ifs -- one for checking boundaries and one for checking visiting and it isn't water.
There was a problem hiding this comment.
Agreed. It's a very long-winded way of writing some simple tests.
There was a problem hiding this comment.
OK, will create a big OR condition...
Case isn't the right thing here, since all are "return" - there are no other options...
| */ | ||
| public class KEMIslandsFinder { | ||
|
|
||
| private boolean[][] m_visisted_map; |
There was a problem hiding this comment.
Is better to name it as mVisitedMap?
| { | ||
| for (int j = -1; j < 2; j++) | ||
| { | ||
| markIslandAsVisited(map,curr_row+i,curr_col+j); |
There was a problem hiding this comment.
Do you check here diagonal elements? Read the task more attentively.
There was a problem hiding this comment.
Yep, only now realised it... thnx
simon-frankau
left a comment
simon-frankau
left a comment
There was a problem hiding this comment.
My one recommendation would be to write more compactly. It's possible to be too terse, but you're not at risk of that currently.
You also need to read the assignment more carefully and tweak the algorithm.
| * gets: a map and the size of the map (rxc) | ||
| * and returns the number of Islands in the | ||
| * map | ||
| */ |
There was a problem hiding this comment.
These lines are wrapped unusually short. People normally wrap around, say, 80 columns, although the Google Java coding style suggests 100 columns.
| return currentNumberOfIslands; | ||
| } | ||
|
|
||
| private void initVisitedMap() |
There was a problem hiding this comment.
This method does nothing, since the boolean arrays are automatically initialised to false.
There was a problem hiding this comment.
Wasn't sure about it, but it passes the tests without it, so I remove it now
| } | ||
|
|
||
| /* Private Helpers */ | ||
| private int countIslands(boolean[][] map, int curr_row, int curr_col) |
There was a problem hiding this comment.
This method could be written a lot more simply as a pair of nested for loops iterating over the rows and columns.
| /* Move to the next location */ | ||
| if ((curr_col+1) < map[curr_row].length) | ||
| { | ||
| currentNumberOfIslands += this.countIslands(map, curr_row, curr_col + 1); |
| if (m_visisted_map[curr_row][curr_col] == true) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Agreed. It's a very long-winded way of writing some simple tests.
| { | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
return (map[curr_row][curr_col] && !m_visited_map[curr_row][curr_col]) ? 1 : 0;
Alternatively, take out the ternary, make this bool isNewIsland(...), and have the caller do "if (isNewIsland(...)) { currentNumberOfIslands++; }"
|
|
||
| public static void tearDownClass() { | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove empty boilerplate methods that aren't needed.
| boolean[][] map = {{true,false},{false,true}}; | ||
| KEMIslandsFinder instance = new KEMIslandsFinder(); | ||
| assertEquals(1, instance.countIslands(row, col, map)); | ||
| } |
There was a problem hiding this comment.
Incorrect behaviour.
There was a problem hiding this comment.
Yep, only now realised it... thnx
| KEMIslandsFinder instance = new KEMIslandsFinder(); | ||
| assertEquals(5, instance.countIslands(row, col, map)); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it would have been good to test an island with a lake inside it (a doughnut island), and I think you misunderstood the details of the assignment so you've got some incorrect expectations in your tests, but it's basically pretty good testing.
There was a problem hiding this comment.
Yes, I missed a word there... thanks
Sure, will add this test
|
Fixed most of the comments, thanks |
problem with rename of a file
problem with rename of a file
problem with rename of a file
problem with rename of a file
for the public methods of the algorithm
3 participants
It has few changes from assignment 3, please ignore these, thanks!