Conversation
hayashi-ay
left a comment
There was a problem hiding this comment.
関係のないファイルも差分に含まれているので、関係あるやつだけ差分に含めるようにして欲しいです
| return row >= 0 && row < grid.size() && column >= 0 && column < grid[0].size(); | ||
| } | ||
|
|
||
| int measure_island(vector<vector<int>>& grid, const int& row, const int& column, int& area) { |
There was a problem hiding this comment.
個人的にはareaは参照で取り回すのではなく、帰り値で受け渡したいです。
| measure_island(grid, row + 1, column, area); | ||
| measure_island(grid, row, column - 1, area); |
There was a problem hiding this comment.
@hayashi-ay
コメントありがとうございます。
不要なファイルすみません。今回は削除→commitで対応しました。
4から2に出来ないかなと操作している作業中のものをPushしておりました🙇
別のコメントに関しては後ほど対応します。
| public: | ||
| const int Water = 0; | ||
| const int Land = 1; | ||
| const int Visited = 2; |
There was a problem hiding this comment.
PythonのPEP8との違いをふと気になって調べてみたのですが、
C++のGoogleのコーディング規約だと定数の頭にkをつけるみたいですね。
https://google.github.io/styleguide/cppguide.html#Constant_Names
何かしらの規約に従って命名されていれば問題なさそうですが、調べたので共有させていただきます。
| return row >= 0 && row < grid.size() && column >= 0 && column < grid[0].size(); | ||
| } | ||
|
|
||
| int measure_island(vector<vector<int>>& grid, const int& row, const int& column, int& area) { |
There was a problem hiding this comment.
int はコピーしてもたいしたコストではないのでコンストリファレンスにする必要はないでしょう。
| const int Visited = 2; | ||
|
|
||
| bool is_inside(vector<vector<int>>& grid, const int& row, const int& column) { | ||
| return row >= 0 && row < grid.size() && column >= 0 && column < grid[0].size(); |
|
|
||
| class Solution { | ||
| public: | ||
| const int Land = 1; |
There was a problem hiding this comment.
定数を定義するとき、 constexpr を使うと C++11 っぽくなると思います。
There was a problem hiding this comment.
@nodchip
レビューありがとうございます。修正しました。
constexpr知りませんでした🙇
https://qiita.com/saltheads/items/dd65935878a0901fe9e7 も読んでみました。
| const int Visited = 2; | ||
|
|
||
| bool is_inside(vector<vector<int>>& grid, const int& row, const int& column) { | ||
| if ((0 <= row && row < grid.size()) && (0 <= column && column < grid[0].size())) { |
There was a problem hiding this comment.
インデントがずれているようです。
また、
return 0 <= row && row < grid.size() && 0 <= column && column < grid[0].size()
のほうがシンプルだと思います。
| const int Land = 1; | ||
| const int Visited = 2; | ||
|
|
||
| bool is_inside(vector<vector<int>>& grid, const int& row, const int& column) { |
There was a problem hiding this comment.
C++ の関数名は UpperCamel をお勧めいたします。
https://google.github.io/styleguide/cppguide.html#Function_Names
Ordinarily, functions should start with a capital letter and have a capital letter for each new word.
また、他のクラスから呼び出されない関数は、 private とするとよいと思います。
|
|
||
| int maxAreaOfIsland(vector<vector<int>>& grid) { | ||
| int max_size = 0; | ||
| int row_size = grid.size(); |
There was a problem hiding this comment.
row_size だと、行の大きさを表しているように感じました。個人的には num_rows がよいと思います。
|
|
||
| grid[row][column] = Visited; | ||
|
|
||
| int x_index = row * column_size + column; |
There was a problem hiding this comment.
x と column がどちらも横方向の座標を表す単語のため、混乱しやすいように感じます。 x_index のほうをcell_index、y_index のほうを neighbor_cell_index などとしてはいかがでしょうか?
| return x; | ||
| } | ||
|
|
||
| void unite(int x, int y) { |
There was a problem hiding this comment.
UnionFind で unite() という関数名は、あまり見ないように思います。 Union() という関数名をよく見かけます。
| } else if (rank[root_x] < rank[root_y]) { | ||
| root[root_x] = root_y; | ||
| area[root_y] += area[root_x]; | ||
| } else { |
There was a problem hiding this comment.
ここを } else if (root_x != root_y) { として、 is_connect() を削除し、呼び出し元では is_connect() を呼ばずに、いきなり unite() を呼び出したほうが、コードがシンプルになると思います。
There was a problem hiding this comment.
uniteの最初にif(root_x == root_y) return をしちゃう方がシンプルだと自分は思いました。
| return area[find(x)]; | ||
| } | ||
|
|
||
| bool is_connect(int x, int y) { |
There was a problem hiding this comment.
対象が複数あるため、 are_connect() のほうが良いかもしれません。
また、 be 動詞のあとの動詞は過去分詞形にして、 are_connected() にするとよいと思います。
|
|
||
| int x_index = row * column_size + column; | ||
| for (const auto& direction : directions) { | ||
| int neighber_row = row + direction.dx; |
| return row >= 0 && row < grid.size() && column >= 0 && column < grid[0].size(); | ||
| } | ||
|
|
||
| int measure_island(vector<vector<int>>& grid, const int& row, const int& column, int& size) { |
There was a problem hiding this comment.
CPU の汎用レジスターのサイズ以下の値を参照経由で渡すと、アドレスの計算が行われるため、遅くなるとされています。値渡しのほうがよいと思います。
Effective C++ 第3版
https://www.maruzen-publishing.co.jp/item/b294734.html
20項 値渡しよりconst参照渡しを使おう
Effective C++ 第3版は一通り読まれることをお勧めいたします。
There was a problem hiding this comment.
@nodchip
以前紹介いただいて読み進めているのですが、まだ10項です...
しっかりと読み進めます🙇
|
|
||
| int find(int x) { | ||
| while (x != root[x]) { | ||
| x = root[x]; |
There was a problem hiding this comment.
rootってこの実装だとおそらく直接の親を指していますよね?
root_nodeは親を持たないnodeのことを指すので、ここでの命名はparentとかにした方がいい気がします
https://ja.wikipedia.org/wiki/%E6%9C%A8%E6%A7%8B%E9%80%A0_(%E3%83%87%E3%83%BC%E3%82%BF%E6%A7%8B%E9%80%A0)
There was a problem hiding this comment.
@nittoco
ご指摘のとおり親です。資料もありがとうございます。
アルゴリズムイントロダクションを読み直したらそちらも親と記載ございました。。。修正しました🙇
| int area = 0; | ||
| max_area = max(measure_island(grid, row, column, area), max_area); |
There was a problem hiding this comment.
自分ならこんな感じで書きます。
int area = measure_island(grid, row, column, 0);
max_area = max(area, max_area);There was a problem hiding this comment.
@hayashi-ay
返信遅くなりました。そうですね。0で初期化する意味ないですね。。。
step6で修正しました。
076f16b
問題へのリンク
695. Max Area of Island
https://leetcode.com/problems/max-area-of-island/description/
問題文(プレミアムの場合)
備考
次に解く問題の予告
https://leetcode.com/problems/number-of-connected-components-in-an-undirected-graph/description/
フォルダ構成
LeetCodeの問題ごとにフォルダを作成します。
フォルダ内は、step1.cpp、step2.cpp、step3.cpp, union_find.cppとmemo.mdとなります。
memo.md内に各ステップで感じたことを追記します。