200.Number of islands #3

haniwachann · 2024-11-13T14:28:28Z

<問題>
https://leetcode.com/problems/number-of-islands/

oda · 2024-11-13T14:54:56Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+    int numIslands(vector<vector<char>>& grid) {
+        row_size = size(grid);
+        column_size = size(grid[0]);
+        recognized_island.assign(row_size, vector<char>(column_size, 0));


0 か '1' が入っているということに違和感ですね。
1 がきっと入ったら混乱しますね。
bool のほうが間違えないのでは。

ちなみに、'1'は49ですね。
https://www.asciitable.com/

Oda さんと同じ感想を持ちました。bool がよさそうです

ありがとうございます。
step4でboolに修正しました。
適切な型を使うことを心がけます。

liquo-rice

スペースの使い方を直してください。early returnを適所に使っている点は良さそうでした。

liquo-rice · 2024-11-13T14:33:31Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+```c++
+class Solution {
+private:
+    int row_size,column_size;


Add a space after the comma
int row_size, column_size;

丁寧にコードを見て頂きありがとうございます。
step4で修正しました。
以下を少しづつ見ていけたらと思っています。
https://google.github.io/styleguide/cppguide.html

liquo-rice · 2024-11-13T14:33:52Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+class Solution {
+private:
+    int row_size,column_size;
+    int num_island=0;


Add spaces around the equal sign
int num_island = 0;

liquo-rice · 2024-11-13T14:34:21Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+private:
+    int row_size,column_size;
+    int num_island=0;
+    vector<vector<char>> recognized_island; //既に島として認識されている点を1とする。


vector<vector<bool>> visited_

liquo-rice · 2024-11-13T14:36:30Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+    int num_island=0;
+    vector<vector<char>> recognized_island; //既に島として認識されている点を1とする。
+
+//start_pointと同じ島である点を調べ上げる。


Indent and align the comment to the function below
Add a space after "//"

// start_pointと同じ島である点を調べ上げる。 void check_same_island (pair<int, int> start_point, vector<vector<char>>& grid) {

liquo-rice · 2024-11-13T14:37:11Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+    vector<vector<char>> recognized_island; //既に島として認識されている点を1とする。
+
+//start_pointと同じ島である点を調べ上げる。
+    void check_same_island (pair<int, int> start_point, vector<vector<char>>& grid) {


const vector<vector<char>>& grid

liquo-rice · 2024-11-13T14:49:44Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+            tie(x_connected, y_connected) = seen.front();
+            recognized_island[x_connected][y_connected] = '1';
+            seen.pop();
+            for (int i　=　0; i　<　size(connected_point); i++) {


for (auto [dx, dy] : deltas) {

liquo-rice · 2024-11-13T14:51:00Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+```c++
+class Solution {
+private:
+    int row_size,column_size;


Add a trailing underscore to differentiate between member and local variables
e.g. row_size_

liquo-rice · 2024-11-13T14:51:29Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+                int x,y;
+                x = x_connected + connected_point[i].first;
+                y = y_connected + connected_point[i].second;
+                if( x < 0 || y < 0 || x >= row_size || y >= column_size){


Fix spacing

if(x < 0 || y < 0 || x >= row_size || y >= column_size) {

liquo-rice · 2024-11-13T14:51:38Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+                if( x < 0 || y < 0 || x >= row_size || y >= column_size){
+                    continue;
+                }
+                if (grid[x][y] == '0' || recognized_island[x][y] == '1'){


Fix spacing

if (grid[x][y] == '0' || recognized_island[x][y] == '1') {

liquo-rice · 2024-11-13T14:52:57Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+                if (grid[i][j] == '0' || recognized_island[i][j] == '1') {
+                    continue;
+                }
+                check_same_island ({i,j}, grid);


Fix spacing

check_same_island({i, j}, grid);

oda · 2024-11-13T14:59:03Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+    int row_size,column_size;
+    int num_island=0;
+    vector<vector<char>> recognized_island; //既に島として認識されている点を1とする。


ここ、クラスメンバーにしておくと、numIslands を複数のスレッドから呼んだときにうまく動かなくなります。
スレッドセーフでない、などといいます。スレッドセーフでないときにはコメントが欲しい気持ちはありますね。

HTML Parser とかだったら、HTML 一つにつきインスタンス一つという設計も自然でしょうが、うーん、この程度の機能でスレッドセーフでないのか、という感覚はあります。
https://discord.com/channels/1084280443945353267/1237649827240742942/1293905886506385438

@oda thread safetyに疎くて申し訳ないのですが、この場合どのように記述することが正解なのでしょうか？

https://source.chromium.org/search?q=%2F%2F.*thread.%3Fsafe%20file:%5C.cc$
好きに書けばいいですが、たとえばこんなのです。

メンバ変数ではなくて、関数内のローカル変数にするのが良いのかなと思います。メンバ変数で持ってしまうと、Soluionインスタンスが複数のスレッドから同時に呼ばれるとメンバ変数が共有されているため意図せず書き換わってしまいます。

ありがとうございます。
使ってくれる人がわかりやすいように書こうとはしているつもりですが、
意識できていない部分でした....

step4としては、hayashi-ayさんの案のとおり、ローカル変数にして、
スレッドセーフとなるように修正しました。
(ローカル変数だと、それぞれのスレッドに対して別々に用意されるスタックに
格納されるので、同時に別のスレッドからインスタンスが呼びだれてもスレッド競合は起きないと理解しています。)
https://www.divx.co.jp/media/techblog-220627

austyhooong · 2024-11-13T16:06:20Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+//start_pointと同じ島である点を調べ上げる。
+    void check_same_island (pair<int, int> start_point, vector<vector<char>>& grid) {
+        queue<pair<int, int>> seen;
+        vector<pair<int ,int >> connected_point = {{1,0},　{-1,0},　{0,1},　{0,1}};


これは定数なので、以下のようにclass member としてrefactorしても良いのかなと思いました

private:
constexpr static int _connected_points[4][2] {
{0, 1}, {0, -1}, {1, 0}, {-1, 0}
};

ありがとうございます。
vectorにconstexprを使おうとしたのですが、動的配列なので、ダメなのですね...
https://stackoverflow.com/questions/33241909/cannot-create-constexpr-stdvector

vectorに対してconstは使えるとのことなので、constで定数化してみました。
const vector<pair<int ,int >> deltas = {{1,0}, {-1,0}, {0,1}, {0,-1}};

austyhooong · 2024-11-13T16:11:13Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+```c++
+class Solution {
+private:
+    int row_size,column_size;


私個人の意見ですが、class memberとlocal variableを区別しやすくするために、class memberには "_"を付けるのが良いかと思います。

ex:
int row_size_, column_size_
or
int _row_size, _column_size

因みに_を先につけると、IDEで_を打つだけで変数を見つけてくれるメリットがあります！

https://google.github.io/styleguide/cppguide.html#Variable_Names

Data members of classes, both static and non-static, are named like ordinary nonmember variables, but with a trailing underscore.

Google Style Guide もそれやってますね。これも趣味の範囲です。(自分一人ならば趣味の問題だが、チームで開発するときには周りに合わせるよ、という態度を取れれば良いということです。)

hayashi-ay · 2024-11-14T02:45:20Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+        while (!seen.empty()) {
+            int x_connected, y_connected;
+            tie(x_connected, y_connected) = seen.front();
+            recognized_island[x_connected][y_connected] = '1';


1とだけみてもなんのことなのか分からないので、定数化してあげると良いと思います。

ありがとうございます。
こちらの変数については、visited_islandという変数名に変えて、
bool型に変えることで、意味がわかるようにしてみました。

hayashi-ay · 2024-11-14T02:56:29Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+//start_pointと同じ島である点を調べ上げる。
+    void check_same_island (pair<int, int> start_point, vector<vector<char>>& grid) {
+        queue<pair<int, int>> seen;
+        vector<pair<int ,int >> connected_point = {{1,0},　{-1,0},　{0,1},　{0,1}};


よく見ると、{0,1}が2つありますね。

hayashi-ay · 2024-11-14T02:57:15Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+        while (!seen.empty()) {
+            int x_connected, y_connected;
+            tie(x_connected, y_connected) = seen.front();
+            recognized_island[x_connected][y_connected] = '1';


実際に訪れてからしかrecognized_islandを書き換えていないので、seenに同一のセルが複数個入るケースがありそうです。

step4で、queueに訪問予定のセルが入ったタイミングで、書き換えるように変更しました

thonda28 · 2024-11-14T10:39:59Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+//start_pointと同じ島である点を調べ上げる。
+    void check_same_island (pair<int, int> start_point, vector<vector<char>>& grid) {
+        queue<pair<int, int>> seen;
+        vector<pair<int ,int >> connected_point = {{1,0},　{-1,0},　{0,1},　{0,1}};


connected_point という変数名から、特定の位置を意味しているように感じました。ここでは今見ているポイントからの移動方向ということかと思うので、directions とかがわかりやすそうです。また単数形だと配列であることがわかりにくそうです。

thonda28 · 2024-11-14T10:42:16Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+
+        seen.push(start_point);
+        while (!seen.empty()) {
+            int x_connected, y_connected;


x, y を使うと座標軸と混ざってややこしいので、個人的には2次元配列では避けるようにしています。

tarinaihitori/leetcode#17 (comment)

ありがとうございます。
一つ前のレビューでいただいた指摘も含めて、
step4では変数名がわかりやすいように修正しました。

thonda28 · 2024-11-14T10:54:45Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+                int x,y;
+                x = x_connected + connected_point[i].first;
+                y = y_connected + connected_point[i].second;
+                if( x < 0 || y < 0 || x >= row_size || y >= column_size){


「範囲外である」ことを表す場合、「範囲内でない」という書き方のほうが個人的には理解しやすいです。

またこれも個人的な感覚かもしれませんが、x について確認してから y について見るといった順番で、かつ1つの変数に対して2つの条件がある場合は、数直線のように小さい方から順に書く順番がわかりやすく感じます。

Suggested change

if( x < 0 || y < 0 || x >= row_size || y >= column_size){

if (!(0 <= x && x < row_size && 0 <= y && y < column_size)) {

step4で否定形の形に書き換えました。
変数の向きについては、私的には変数を左にしたほうが読みやすく感じでおります。
seal-azarashi/leetcode#17 (comment)

thonda28 · 2024-11-14T10:55:18Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+    int numIslands(vector<vector<char>>& grid) {
+        row_size = size(grid);
+        column_size = size(grid[0]);
+        recognized_island.assign(row_size, vector<char>(column_size, 0));


Oda さんと同じ感想を持ちました。bool がよさそうです

thonda28 · 2024-11-14T10:59:28Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+                    continue;
+                }
+                check_same_island ({i,j}, grid);
+                ++num_island;


ここで前置インクリメントしているのには理由がありますか？

他の箇所と統一がなかったです。
step4では前置のインクリメントで統一しました。

liquo-rice · 2024-11-15T07:28:24Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+    int _row_size, _column_size;
+    int _num_island = 0;


これらの変数がオブジェクト内で共有されているのでまだスレッドセーフではないです。

また、numIslands()が複数回呼ばれるとどうなりますか？

ありがとうございます。

変な質問かもしれず申し訳ないのですが、
メンバ関数のなかで、メンバ変数にアクセスするとスレッドセーフな関数にはならないと思うのですが、
そうなるとメンバ変数の利点は何なのでしょうか...

私としては、引数にせずに、クラスの様々なメンバ関数からアクセスできるから、コードが書きやすくなると思っていました。

例えば、複数回の関数呼び出しに渡って状態を保存したいなら必要です。スレッドセーフにするには、ロックを使う方法などがあります。https://github.com/haniwachann/leetcode/pull/3/files/a799a8e7598e51d24022fd17ca1eeee8ad11406e#r1841490546

ここでは、int _row_size, _column_size;はgridから取り出せるので、私ならメンバーにしません。_num_islandはそもそもnumIslands()内でしか使われていないです。

liquo-rice · 2024-11-15T07:38:28Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+        vector<vector<bool>> visited_island; // 既に島として認識されている点を1とする。
+        _row_size = grid.size();
+        _column_size = grid[0].size();
+        visited_island.assign(_row_size, vector<bool>(_column_size, 0));


0, 1ではなく、true/falseを使ってください。

liquo-rice · 2024-11-15T07:40:49Z

05_Graph,BFS,DFS/200_Number_of_Islands.md

+};
+```
+
+# step4


スペースの空け方がまだところどころおかしいです。

haniwachann added 4 commits November 13, 2024 06:18

200 Number of islands

2228cb5

問題修正

bb0d8a8

スペース修正

3f2c0ab

スペース修正#2

c39d7d9

oda reviewed Nov 13, 2024

View reviewed changes

liquo-rice reviewed Nov 13, 2024

View reviewed changes

oda reviewed Nov 13, 2024

View reviewed changes

austyhooong reviewed Nov 13, 2024

View reviewed changes

hayashi-ay reviewed Nov 14, 2024

View reviewed changes

thonda28 reviewed Nov 14, 2024

View reviewed changes

#step4を追加

a799a8e

liquo-rice reviewed Nov 15, 2024

View reviewed changes

colorbox mentioned this pull request Dec 11, 2024

200. Number of Islands colorbox/leetcode#31

Merged

	if( x < 0 \|\| y < 0 \|\| x >= row_size \|\| y >= column_size){
	if (!(0 <= x && x < row_size && 0 <= y && y < column_size)) {

+              };
+              ```
+              # step4

200.Number of islands #3

Are you sure you want to change the base?

200.Number of islands #3

Uh oh!

Conversation

haniwachann commented Nov 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liquo-rice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liquo-rice Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austyhooong Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austyhooong Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liquo-rice Nov 13, 2024 •

edited

Loading

austyhooong Nov 13, 2024 •

edited

Loading

austyhooong Nov 13, 2024 •

edited

Loading