200. Number of islands by philip82148 · Pull Request #12 · philip82148/leetcode-swejp

philip82148 · 2025-02-08T14:51:33Z

問題

https://leetcode.com/problems/number-of-islands/description/

補足

<番号>r.cppは<番号>.cppを書きなおしたものになっています。(微妙に違うか同じです)

oda · 2025-02-08T15:52:36Z

+  int numIslands(const vector<vector<char>>& grid) {
+    int num_rows = grid.size(), num_cols = grid[0].size();
+    vector<vector<bool>> seen(num_rows, vector<bool>(num_cols));
+    auto WalkIslands = [&](int start_row, int start_col) {


私の感覚、この lambda は長いので private method にしますね。引数の引き回しが面倒という書く方の都合は、まあ、分かります。
https://google.github.io/styleguide/cppguide.html#:~:text=It%27s%20possible%20for%20use%20of%20lambdas%20to%20get%20out%20of%20hand%3B%20very%20long%20nested%20anonymous%20functions%20can%20make%20code%20harder%20to%20understand.

C++だとメモリ等低レイヤを気にする言語だから、キャプチャした変数をしっかり追いたいことが多いし、その結果複雑なラムダ式は避けたいみたいなプラクティスですかね。(そしてTypeScriptでは良く使われる)
メソッドや関数と違い、その関数内でしか使わない処理を閉じ込められるのがProsの一つだと思ってるんですが(載っていない)。

これはポエム的な感想なんですが、ここって(みんなが理解できる言語はコミュニティによって作られるし、例えば時代によってコミュニティの認識の平均が変われば複雑なクロージャもC++でも当たり前になるかもしれない)言語ゲーム的側面なのかなと思っています。

長すぎる lambda は、読むにあたり認知負荷が高く、読みにくいからだと思います。

あ、ちなみに後半はレビューコメントというより僕の思想(ポエム)なので、以下は読まれなくてもよいのですが、一応詳細を書いておきます。

僕が言っているのは認知負荷が高いというのは一体何が決めているのかということです。
日本語でもプログラミングでも、どれだけ文法的に正しかろうと、見慣れているかパターンかどうかは可読性の高さを左右するファクターだと思うんです。
(#1 (comment) #1 (comment) ここらへんで僕が質問をしている理由と同じです。)

ラムダだって(C++では)インデントが一個違うのとキャプチャした変数が何かが分かりにくいことぐらいしか違いがないと思っていて。
スタイルガイドが推奨するように明示的キャプチャを使えばメソッドとラムダ式の違いはインデントぐらいしかなくなってしまいます(あとはその外側の関数が長くなること)。
TypeScriptではクロージャをよく使いますが、C++ではあまり推奨されないというプラクティス、つまり読みづらさが変わる理由は、もちろん色々個々の理由を並べ立てることは出来ますが、結局言語ゲームなんだろうなと思ったということです。

みんなが理解できる言語はコミュニティによって作られるし、例えば時代によってコミュニティの認識の平均が変われば複雑なクロージャもC++でも当たり前になるかもしれない

というのは、インデント1個の違いと外側の関数が長くなることぐらいしか(メソッドとの)違いがないC++のラムダを多くのC++erが見慣れる日が来れば、このプラクティスも変わるかもしれないということですね。
(これは感想であり、あえて複雑なラムダを使いたいという意図ではございません)

結局は言語ゲームという点については同意です。あくまで個人の考えとなりますが、ソフトウェアエンジニアというペルソナを想定し、そのペルソナに何らかの入力を与えたときに、どのような出力を返すかを想定するということになると思います。そのペルソナを理解し、ペルソナを用いて脳内シミュレーションを行い、ペルソナから良い出力が得られる入力を提示するのが良いと思います。
認知負荷に関しては、自分自身が読んでいて脳が疲れるという感覚を感じるため、きっと他のソフトウェアエンジニア(のペルソナ)も疲れると感じるだろう、という意図で書きました。

追加で言うと、コードレビューを通して、他の人の反応を見て、ペルソナを補正し続けるのが良いと思います。

読みやすさが、コードのみで決まらず、人にも依存するということならそうです。レビューで、「個人的には」「選択肢が見えているか」などと繰り返し言っているのはそうだからです。(このあたりは敷衍して欲しいところですが、反対方向に思い込んでいると演繹能力、読解力の問題で難しいところでしょう。)

確かにここを理解していないと「自分の意見も論理的で正しいはず。」「分かんないのー。」と言い続けることになり、その事自体が強くネガティブに扱われるので、そう理解したほうがいいでしょうね。

ラムダの場合、フローの真ん中に入る、依存関係が不明瞭になる、後ろに書かれたコードが呼べないので書き換えにくいなどの理由がありそうです。少なくとも全プロジェクトを関数一つで残りはラムダにしようとはならないでしょう。

oda · 2025-02-08T15:54:36Z

+// 11分
+// <疑問・不安点(・個人的な感想、違う意見があれば教えてほしいもの)>
+// - 僕はTypeScriptをよく使っていて、クロージャーが好きである。
+//   C++でも使いまくりたいと思っているので、使ってみた。


複雑な Lambda は読みづらくなるのと、どうせ他から呼びたくなってリファクタリングの手間を書けることになるので private メソッドにしますね。

なるほどです。教えてくださりありがとうございます！

colorbox · 2025-02-08T16:57:54Z

+    int num_islands = 0;
+    for (int i = 0; i < num_rows; ++i) {
+      for (int j = 0; j < num_cols; ++j) {
+        if (grid[i][j] == '0' || visited[i][j]) continue;


'0'がマジックナンバーなので定数か何かで持つほうが読みやすくなりそうです。

colorbox · 2025-02-08T16:59:20Z

+        seen[row][col] = true;
+        static constexpr pair<int, int> kDelta[4] = {
+            {1, 0}, {-1, 0}, {0, 1}, {0, -1}};
+        for (auto [dr, dc] : kDelta) {


変数名は略さないほうが読みやすいと思います。

colorbox · 2025-02-08T17:00:08Z

+          positions.pop();
+          if (visited[row][col]) continue;
+          if (grid[row][col] == '0') continue;
+          visited[row][col] = true;


visited判定している直後に置くほうが自然に思えます。

確かにそうですね。ありがとうございます！

colorbox · 2025-02-08T17:00:31Z

+
+        stack<pair<int, int>> positions;
+        positions.push({i, j});
+        while (!positions.empty()) {


ネストが深すぎるので、関数に切り出すと読みやすくなると思います。

t0hsumi · 2025-02-09T00:42:23Z

+        if (seen[row][col]) continue;
+        seen[row][col] = true;
+        static constexpr pair<int, int> kDelta[4] = {
+            {1, 0}, {-1, 0}, {0, 1}, {0, -1}};
+        for (auto [dr, dc] : kDelta) {
+          int new_row = row + dr, new_col = col + dc;
+          if (0 <= new_row && new_row < num_rows && 0 <= new_col
+              && new_col < num_cols && grid[new_row][new_col] == '1') {
+            positions.emplace(new_row, new_col);


自分だったらL29のseen判定を、L35-L36の土地判定に持って行きます。「start_row, start_colから始まる、まだ訪ねていない土地を次に歩きます」と一箇所にしたい気持ちがあります。

ただ、L29のように、seenがfalseであることを確認して、trueにするというのもわかりやすさがあるので、好みかもしれません。

コメントありがとうございます！
ちなみに、そういうパターンもありだと思うのですが、途中でseenが書き変わることがあるので、僕はL29のようにしたいなと考えている感じです。

例えば、グラフが4マスしかなくて、(0,0)からスタートしてDFSする場合、最初に(0,1)、(1,0)をプッシュしますが、(0,1)をDFSするうちに(1,0)もプッシュすることになります。
すると、(1,0)が2回プッシュされているので、2回歩く(seen[row][col] = true;)することにならないでしょうか？
※pushすると同時にseen[row][col] = true;すればいいですが、(ここは解釈次第ですが)そうなるとまだ歩いていないのにseen[row][col] = true;みたいなことになります。

僕の方法だと、とりあえずプッシュしておいて、歩く前にseenしたかどうか確認しているので、2回歩くことを防げます。
※今回の場合歩いているだけなので問題ありませんが、歩く+何かの動作をするとかだと、問題が出てきます。

確かに、今いる島の大きさを数える場合でも、もともと書かれているやり方の方が素直な気がしてきました。

olsen-blue · 2025-02-09T08:58:05Z

+    for (int i = 0; i < num_rows; ++i) {
+      for (int j = 0; j < num_cols; ++j) {
+        if (grid[i][j] == '0' || seen[i][j]) continue;
+        WalkIslands(i, j);


traverse という表現もあるようです。
olsen-blue/Arai60#17 (comment)

なるほど！勉強になります！
ありがとうございます！

nodchip · 2025-02-16T00:34:41Z

+
+// <時間>
+// 11分
+// <感想>


感想を書いていただいたおかげで、思考過程が分かりやすくなり、レビューがしやすくなりました。ありがとうございます。

nodchip · 2025-02-16T00:36:50Z

+class Solution {
+ public:
+  int numIslands(vector<vector<char>>& grid) {
+    int num_rows = grid.size(), num_cols = grid[0].size();


個人的には、 1 行当たりの情報量が多くなると認知負荷が高くなるように感じるため、 1 つの行で複数の変数を宣言しないようにしています。ただ、好みの問題かもしれません。

nodchip · 2025-02-16T00:39:00Z

+    int num_rows = grid.size(), num_cols = grid[0].size();
+    vector<vector<bool>> visited(num_rows, vector<bool>(num_cols));
+    int num_islands = 0;
+    for (int i = 0; i < num_rows; ++i) {


行列の計算以外で、行方向を i、列方向を j とおくのはあまり見ないように思います。 num_rows, num_cols でバウンドしているため、 row, col と置いたほうが分かりやすいと思います。

なるほどです！ありがとうございます！

nodchip · 2025-02-16T00:41:03Z

+        ++num_islands;
+
+        stack<pair<int, int>> positions;
+        positions.push({i, j});


positions.emplace_back(i, j) と書いてもよいと思います。読みやすさはあまり変わらないと思います。処理効率は、型のサイズが小さいため、あまり変わらないかもしれません。生成されるアセンブラを眺めてもよいと思います。

nodchip · 2025-02-16T00:43:22Z

+  int numIslands(const vector<vector<char>>& grid) {
+    int num_rows = grid.size(), num_cols = grid[0].size();
+    vector<vector<bool>> seen(num_rows, vector<bool>(num_cols));
+    auto WalkIslands = [&](int start_row, int start_col) {


長すぎる lambda は、読むにあたり認知負荷が高く、読みにくいからだと思います。

philip82148 added 2 commits February 8, 2025 23:47

add 1-3.cpp

75b76c8

rename 3.cpp -> 2r.cpp

5a30ab6

oda reviewed Feb 8, 2025

View reviewed changes

colorbox reviewed Feb 8, 2025

View reviewed changes

t0hsumi reviewed Feb 9, 2025

View reviewed changes

philip82148 mentioned this pull request Feb 9, 2025

392. Is Subsequence usatie/leetcode#5

Open

olsen-blue reviewed Feb 9, 2025

View reviewed changes

nodchip reviewed Feb 16, 2025

View reviewed changes

Conversation

philip82148 commented Feb 8, 2025

問題

補足

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants