Unit tests to be done #27
Conversation
ssancho2
left a comment
ssancho2
left a comment
There was a problem hiding this comment.
Looks good!, some comments.
| const int cols = 5; | ||
|
|
||
| // A function to check if a given cell (row, col) can be included in DFS | ||
| int isValid(int mat[rows][cols], int row, int col, bool visited[rows][cols]) |
There was a problem hiding this comment.
It will be better if the code can have matrices of different sizes. This could be done having three parameters: int **mat, int num_rows, int num_cols, or preferably using a vector<vector<int>>.
For example, this function could become:
bool isValid(const vector<vector<bool>> &mat, int row, int col, const vector<vector<bool>> &visited)
{
// row number is in range, column number is in range and value is 1
// and not yet visited
return (row >= 0) && (row < mat.size()) &&
(col >= 0) && (col < mat[row].size()) &&
(mat[row][col] && !visited[row][col]);
}Marking them as a const & is not strictly needed, but without them the compiler will make a copy of the matrices every time we call this functions, which is a bit slow.
|
|
||
| // The main function that returns count of islands in a given boolean | ||
| // 2D matrix | ||
| int countIslands(int mat[rows][cols]) |
There was a problem hiding this comment.
Minor detail, but mat should be a bool 2D matrix, as the comment says. :)
| visited[row][col] = true; | ||
|
|
||
| // Recur for all connected neighbours | ||
| for (int k = 0; k < 8; ++k) |
There was a problem hiding this comment.
It may be slightly easier to have two cycles:
for (int dx : {-1, 0, 1}) {
for (int dy : {-1, 0, 1}) {
...
}
}or using for (int dx = -1; dx <= 1; dx++) { ... }
Note that isValid will return false when dx==0 & dy==0, since that cell is already painted as visited.
But since we need to iterate over just 4 neighbours is probably easier as you did :)
| // check all 8 neighbours | ||
| void DFS(int mat[rows][cols], int row, int col, bool visited[rows][cols]) | ||
| { | ||
| // These arrays are used to get row and column numbers of 8 neighbours |
There was a problem hiding this comment.
Note that the problem statement specifies that we do not take diagonal connections.
ssancho2
left a comment
ssancho2
left a comment
There was a problem hiding this comment.
Looks great! Some minor comments.
| { | ||
| this->grid = grid; | ||
| this->rows = grid.size(); | ||
| this->cols = grid[0].size(); |
There was a problem hiding this comment.
Minor comment, grid rows can potentially be of different size, ideally you want to either check that they are all of the same size or handle the case where they are not.
| }; | ||
|
|
||
| // A function to check if a given cell (row, col) can be included in DFS | ||
| bool Islands::isValid(const std::vector<std::vector<bool>> &mat, int row, int col, int rows, int cols, const std::vector<std::vector<bool>> &visited) |
There was a problem hiding this comment.
You don't need to pass cols & rows, they are fields of the class, right?
| int Islands::countIslands(const std::vector<std::vector<bool>> &mat, int rows, int cols) | ||
| { | ||
| // Make a bool array to mark visited cells. | ||
| // Initially all cells are unvisited |
There was a problem hiding this comment.
Indentation is a bit inconsistent here.
| private: | ||
| std::vector<std::vector<bool>> grid; | ||
| public: | ||
| int rows; |
There was a problem hiding this comment.
Why rows & cols are public?
| int rows; | ||
| int cols; | ||
| Islands(std::vector<std::vector<bool>> grid); | ||
| bool isValid(const std::vector<std::vector<bool>> &mat, int row, int col, int rows, int cols, const std::vector<std::vector<bool>> &visited); |
There was a problem hiding this comment.
isValid() & DFS() could also be private. They are not useful for a user, right? They are part of the implementation of countIslands()
|
|
||
| } | ||
|
|
||
| void tets_noWater() |
There was a problem hiding this comment.
test -> test :)
Also, this is all water, that's why the result is 0.
|
|
||
| } | ||
|
|
||
| void test_allWater() |
No description provided.