Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions 31-45/37.Number-of-Islands/1.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#if __has_include("../../debug.hpp")
#include "../../debug.hpp"
#endif
// ここまでローカルでのデバッグ用なので気にしないでください --------------------

#include <stack>
#include <vector>

using namespace std;

// <時間>
// 11分
// <感想>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 競プロで書きまくっているBFS/DFSであるが、エンジニアにとって
// 読みやすいパターンは何か調べなければならない。
// なお、久しぶりで書くのに時間がかかってしまったが、
// その他のパターンもいくつか見えている状態なので反復は一旦スキップ
// <他の人のレビューを読んでコメント>
// - row, colの命名について。こういうのが欲しかった。
// https://github.com/olsen-blue/Arai60/pull/17/files#r1931573071
// - 4方向のpushをどう処理するか
// https://github.com/katataku/leetcode/pull/16/files#r1905332545
// - 範囲内の判定をどうするか
// https://github.com/Hurukawa2121/leetcode/pull/17/files#r1898908077
// - 意外とC++でクロージャ使うのありなんだな
// https://github.com/Hurukawa2121/leetcode/pull/17/files#r1898590021
// - 競プロで見慣れ過ぎているパターンなので違和感がなかったが、
// BFS部分は別関数にしないと読みづらいだろうか?
// - walkいいな
// - pairやtupleを使わなくてもいいケース
// https://github.com/colorbox/leetcode/pull/31/files#r1881109902
// - chromium code searchではvisitedとseenの両方を確認
// - コーディングに対する根本的な何かをつかんだ気がする
// - pairの場合emplaceもpushも同じ結果になると思っているのだけど、
// 違うかな。まあemplaceの方が無難か
class Solution {
public:
int numIslands(vector<vector<char>>& grid) {
int num_rows = grid.size(), num_cols = grid[0].size();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vector<vector<bool>> visited(num_rows, vector<bool>(num_cols));
int num_islands = 0;
for (int i = 0; i < num_rows; ++i) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

++num_islands;

stack<pair<int, int>> positions;
positions.push({i, j});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

while (!positions.empty()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

auto [row, col] = positions.top();
positions.pop();
if (visited[row][col]) continue;
if (grid[row][col] == '0') continue;
visited[row][col] = true;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (row + 1 < num_rows) positions.push({row + 1, col});
if (row - 1 >= 0) positions.push({row - 1, col});
if (col + 1 < num_cols) positions.push({row, col + 1});
if (col - 1 >= 0) positions.push({row, col - 1});
}
}
}
return num_islands;
}
};
56 changes: 56 additions & 0 deletions 31-45/37.Number-of-Islands/2.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#if __has_include("../../debug.hpp")
#include "../../debug.hpp"
#endif
// ここまでローカルでのデバッグ用なので気にしないでください --------------------

#include <stack>
#include <vector>

using namespace std;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// (が、避けた方がいいだろうか)
// - rowとcolが思いっきりshadowingしているけど、
// 別に良くないかと思ってあえて使っている(良くないか)
class Solution {
public:
int numIslands(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 walkIsland = [&](int row, int col) {
stack<pair<int, int>> positions;
positions.emplace(row, col);
while (!positions.empty()) {
auto [row, col] = positions.top();
positions.pop();
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 [d_row, d_col] : kDelta) {
int next_row = row + d_row, next_col = col + d_col;
if (!(0 <= next_row && next_row < num_rows && 0 <= next_col
&& next_col < num_cols)) {
continue;
}
if (grid[next_row][next_col] == '0') continue;
positions.emplace(next_row, next_col);
}
}
};

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' || seen[i][j]) continue;
walkIsland(i, j);
++num_islands;
}
}
return num_islands;
}
};
53 changes: 53 additions & 0 deletions 31-45/37.Number-of-Islands/2r.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#if __has_include("../../debug.hpp")
#include "../../debug.hpp"
#endif
// ここまでローカルでのデバッグ用なので気にしないでください --------------------

#include <stack>
#include <vector>

using namespace std;

// <時間>
// 6分
// <疑問・不安点(・個人的な感想、違う意見があれば教えてほしいもの)>
// - dr, dcって伝わるかな
// - new_という命名が悪いとは言わないが、
// この場合個人的にnew_row, new_colはダサさ(初心者感)を感じるのだが
// 気にし過ぎかな
class Solution {
public:
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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

私の感覚、この 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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

stack<pair<int, int>> positions;
positions.emplace(start_row, start_col);
while (!positions.empty()) {
auto [row, col] = positions.top();
positions.pop();
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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Comment on lines +29 to +37
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントありがとうございます!
ちなみに、そういうパターンもありだと思うのですが、途中で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回歩くことを防げます。
※今回の場合歩いているだけなので問題ありませんが、歩く+何かの動作をするとかだと、問題が出てきます。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
};

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' || seen[i][j]) continue;
WalkIslands(i, j);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

++num_islands;
}
}
return num_islands;
}
};