200. Number of Islands by colorbox · Pull Request #31 · colorbox/leetcode

colorbox · 2024-12-11T15:09:02Z

https://discord.com/channels/1084280443945353267/1183683738635346001/1194347590125367467
https://discord.com/channels/1084280443945353267/1200089668901937312/1201114006631501875
haniwachann/leetcode#3
tarinaihitori/leetcode#17
hroc135/leetcode#17
seal-azarashi/leetcode#17
thonda28/leetcode#15
goto-untrapped/Arai60#38
Ryotaro25/leetcode_first60#18
nittoco/leetcode#23
Yoshiki-Iwasa/Arai60#16
Mike0121/LeetCode#34
tshimosake/arai60#8
kazukiii/leetcode#18
TORUS0818/leetcode#19
fhiyo/leetcode#20
sakupan102/arai60-practice#17
rossy0213/leetcode#8
ryoooooory/LeetCode#2
https://hayapenguin.com/notes/LeetCode/200/NumberOfIslands

oda · 2024-12-11T22:29:14Z

200/step2.cpp

+  int height;
+  int width;
+  vector<vector<bool>> checked;


これをメンバ変数で持つと、numIslands のスレッド並列性が失われます。失っては絶対にいけないわけではないですが、機能の複雑性とのバランスを考えたときに失うほどのものかとは思います。
https://discord.com/channels/1084280443945353267/1237649827240742942/1265223555013152891

ありがとうございます、スレッド安全性については見えていませんでした。

oda · 2024-12-11T22:32:20Z

200/step1.cpp

+        CheckIsland(coordinate, grid);
+        ++count;
+      }
+      visited.insert(coordinate);


これ、CheckIsland の中でも insert されているので不要でしょうか。

たしかにこれは不要でした・・・。

oda · 2024-12-11T22:34:23Z

200/step1.cpp

+
+まずBFSな方法を思いつく。
+島を探索し、陸を見つけたら関数を呼び出して陸地すべてを探索済みにしてカウントアップする。
+初期値として{0, 0}を設定していたが、そこが島だと適切にカウントされないパターンがあったためすべてのマスをまずキューに入れたらpassした


分かってそうですが、二重の BFS にしたわけですが、外側の BFS は全体を舐めているだけなので、ただのループで構わないですね。

oda · 2024-12-11T22:39:46Z

200/step1.cpp

+  struct Coordinate {
+    int x;
+    int y;
+    bool operator<(const Coordinate& other) const {


私はこういうの pair や tuple でもういいかなあと思っています。
というのも、C++17 からですが structured binding があり、
pair にして

auto [row, col] = next_coordinates.top();

などと書けるようになっていて十分に意図が伝わるからです。

なるほど、pairやtupleを意図的に避けていましたがstructure bindingでかなりわかりやすくできそうですね。
ありがとうございます。

oda · 2024-12-11T22:43:08Z

200/step1.cpp

+      if (grid.at(coordinate.y).at(coordinate.x) == '1') {
+        visited.insert(coordinate);
+        if (coordinate.x - 1 >= 0) island_coordinates.push({coordinate.x - 1, coordinate.y});
+        if (coordinate.x + 1 < width) island_coordinates.push({coordinate.x + 1, coordinate.y});
+        if (coordinate.y - 1 >= 0) island_coordinates.push({coordinate.x, coordinate.y - 1});
+        if (coordinate.y + 1 < height) island_coordinates.push({coordinate.x, coordinate.y + 1});
+      }


私はこれ if continue でインデント下に下げます。

oda · 2024-12-11T22:56:46Z

200/step1.cpp

+        if (coordinate.x - 1 >= 0) island_coordinates.push({coordinate.x - 1, coordinate.y});
+        if (coordinate.x + 1 < width) island_coordinates.push({coordinate.x + 1, coordinate.y});
+        if (coordinate.y - 1 >= 0) island_coordinates.push({coordinate.x, coordinate.y - 1});
+        if (coordinate.y + 1 < height) island_coordinates.push({coordinate.x, coordinate.y + 1});


ここの部分は先にチェックするか一回 queue に入れて、後からチェックするかでしょう。
繰り返しっぽいので範囲チェックを lambda にする手もあります。

oda · 2024-12-11T22:59:16Z

200/step2.cpp

+    stack<Coordinate> target_coordinates;
+    target_coordinates.push({col, row});
+    while (!target_coordinates.empty()) {
+      Coordinate coordinate = target_coordinates.top();


一回、row, col を const int で宣言してしまってから、書いたほうがいい気がしますね。
ここ以下、row col が主役なのに、coordinate.{row,col} で呼んでいるのは冗長です。

thonda28 · 2024-12-12T10:17:30Z

200/step1.cpp

+    for (int y = 0; y < grid.size(); ++y) {
+      for (int x = 0; x < grid.front().size(); ++x) {
+        checking_coordinates.push({x, y});
+      }
+    }


ここの x, y は x 軸と y 軸の方向を意味しているかと思いますが、ややこしくなりがちなので後の Step で書いているように row, col のような書き方のほうが誤解が少なそうです。

ただ row, col を使う場合、その最大値として height, width を使うのは違和感があります。height は下から上に increment されるイメージがあるためです。

for (int row = 0; row < height; ++row) { for (int col = 0; col < width; ++col) {

height, width を避けるとなると、問題文中で与えられている m, n または num_rows, num_cols あたりが候補に出てくるかと思います。

コメントありがとうございます。
たしかに、columnとrowに名前を寄せるのは直感的で良さそうです。

thonda28 · 2024-12-12T10:30:29Z

200/step2.cpp

+    while (!target_coordinates.empty()) {
+      Coordinate coordinate = target_coordinates.top();
+      target_coordinates.pop();
+      if (coordinate.col < 0 || width <= coordinate.col || coordinate.row < 0 || height <= coordinate.row) {


「範囲外である」という条件ではなく「範囲内でない」という条件で書くほうが個人的にはわかりやすいです。またこちらで Oda さんも言ってますが、範囲内を求める lambda 式を定義するのもよさそうです

step1と違い、同様のifが複数回出てこないので、lambdaにする利点少なさそうです。
lambdaにすることで名前をつけれて可読性が高まるという利点がありそうですが、そのために複数行にする必要はないかと思います。

thonda28 · 2024-12-12T10:38:02Z

200/step3.cpp

+      next_coordinates.emplace(coordinate.row + 1, coordinate.col);
+      next_coordinates.emplace(coordinate.row - 1, coordinate.col);
+      next_coordinates.emplace(coordinate.row, coordinate.col + 1);
+      next_coordinates.emplace(coordinate.row, coordinate.col - 1);


ここはループで定義するのも1つですね。4行程度であれば気にならないかもですが、仮に斜めも含む8方向を探索するような仕様変更があった場合、似たコードが4行増えてさらに冗長になってしまいそうです。

仮に斜めも含む8方向を探索するような仕様変更があった場合

それはいわゆる「早すぎる最適化」なので私は実際にその仕様が発生した時にそのようにするのが良いかと思います。
ループ定義するのも良さそうですが、現状はこのままのほうがシンプルそうです。

austyhooong · 2024-12-12T14:14:07Z

200/step1.cpp

+    queue<Coordinate> checking_coordinates;
+    for (int y = 0; y < grid.size(); ++y) {
+      for (int x = 0; x < grid.front().size(); ++x) {
+        checking_coordinates.push({x, y});


austyhooong · 2024-12-12T14:19:51Z

200/step3.cpp

+  }
+
+ private:
+  const char sea = '0';


constexpr static char sea_ = '0';

constexprとstaticは良さそうですね、ありがとうございます。
でも変数名はseaのままにします。

austyhooong · 2024-12-12T14:20:50Z

200/step3.cpp

+    int col;
+  };
+
+  void WalkThroughIsland(int row, int col, vector<vector<char>>& grid) {


const vector<vector>& grid

const vector<vector<char>>& gridでしょうか？
たしかにここはconstつけるほうが良さそうですね

austyhooong · 2024-12-12T14:23:41Z

200/step3.cpp

+      if (coordinate.row < 0 || coordinate.row >= height || coordinate.col < 0 || coordinate.col >= width) {
+        continue;
+      }
+      if (grid.at(coordinate.row).at(coordinate.col) == sea || visited.at(coordinate.row).at(coordinate.col)) {


ここは関数化しても良いかと思いました！

auto check_boundary = [&](int row, int col) {
return row < 0 || row >= height && col < 0 || col >= width || grid[row][col] == sea || visited[row][col]
};

繰り返し登場しない処理で、lambdaにする利点がないのでこのままのほうが読みやすいと思いました。

nodchip · 2024-12-16T14:04:40Z

200/step1.cpp

+      if (visited.contains(coordinate)) {
+        continue;
+      }
+      if (grid.at(coordinate.y).at(coordinate.x) == '1') {


自分は gird[coordinate.y][coorinamte.x] と書くことが多いです。 at() を使うことはあまりありません。

[] と at() の違いについては
konnysh/arai60#6 (comment)
が参考になると思います。

ありがとうございます。
[]だと未定義動作を起こす可能性があるため、atを利用した方が安全と考えこのようにしていました。

nodchip · 2024-12-16T14:06:55Z

200/step1.cpp

+        ++count;
+      }
+      visited.insert(coordinate);
+      if (coordinate.x - 1 >= 0) checking_coordinates.push({coordinate.x - 1, coordinate.y});


上下左右方向の座標の差分を表す配列と、範囲外にアクセスしようとしているかどうかを判定する関数を用意し、ループで方向を回して処理したほうが、記述の重複なく、シンプルに読みやすく書けると思います。

200. Number of Islands

68e0041

oda reviewed Dec 11, 2024

View reviewed changes

thonda28 reviewed Dec 12, 2024

View reviewed changes

austyhooong reviewed Dec 12, 2024

View reviewed changes

nodchip reviewed Dec 16, 2024

View reviewed changes

colorbox merged commit b672ad8 into main Feb 1, 2025

colorbox deleted the 200 branch February 1, 2025 05:12

Conversation

colorbox commented Dec 11, 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!

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!

thonda28 Dec 12, 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 Dec 12, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thonda28 Dec 12, 2024 •

edited

Loading

austyhooong Dec 12, 2024 •

edited

Loading