200. Number of Islands by seal-azarashi · Pull Request #17 · seal-azarashi/leetcode

seal-azarashi · 2024-08-27T01:28:36Z

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

nodchip · 2024-08-27T09:24:36Z

arai60/Graph_BFS_DFS/number-of-islands.md

+class Solution {
+    public int numIslands(char[][] grid) {
+        int numberOfIslands = 0;
+        boolean[][] traversed = new boolean[grid.length][grid[0].length];


step2 のように下の行と入れ替えて、
boolean[][] traversed = new boolean[m][n];
としたほうがシンプルになると思います。

nodchip · 2024-08-27T09:25:01Z

arai60/Graph_BFS_DFS/number-of-islands.md

+        int n = grid[0].length;
+        for (int i = 0; i < m; i++) {
+            for (int j = 0; j < n; j++) {
+                if (grid[i][j] == '0' || traversed[i][j] == true) {


if (grid[i][j] == '0' || traversed[i][j]) { のほうがシンプルだと思います。

nodchip · 2024-08-27T09:29:25Z

arai60/Graph_BFS_DFS/number-of-islands.md

+    }
+
+    private void traverseAdjacentCells(char[][] grid, boolean[][] traversed, int m, int n, int i, int j) {
+        boolean isOutOfBounds = i < 0 || i >= m || j < 0 || j >= n;


boolean isInside = 0 <= i && i < m && 0 <= j && j < n; if (!isInside || grid[i][j] == '0' || traversed[i][j]) { return; }

としたほうが読みやすく感じます。

不等号の式の左右の方向は、比較されるほうを左に持ってくる派と、数直線上に一直線上に並べる派がいるように思います。実際の現場においては、チームのやり方に合わせることをお勧めします。

また、一般に、変数には肯定的な意味合いを持たせ、式の中で ! を使って否定したほうが読みやすくなると思います。今回の場合とは違うのですが、否定的な意味合いの変数を ! を使って否定すると、二重否定による肯定になり、認知負荷が上がり、読みにくく感じます。

ありがとうございます。

肯定的な意味合いの単語で構成され、判定の際に否定演算子を付与した上記の例の方が見やすく感じました。

また、たしかに数直線上に一直線上に並べる方が読みやすく感じました。今回特に意識せずに書いたら比較される方を左にしていましたが、仰る通り実際の業務ではチームのやり方に合せられればと思います。

ちなみに、どちらの書き方にするかについて、これまでの仕事においてチームで決まってたことってありましたか？
今までこれについて決めていたチームを見たことがなかったので、外資の大きい企業ではそういうこともあるのかな、と興味本位で気になりました。もし心当たりあれば参考までに教えて頂けると嬉しいです。

コードレビューでなんでそうしたのー、って聞かれたことはありますね。

チームで決まっていたことはなかったように思います。 G 社時代に、自分が数直線上に並べる書き方をしていたところ、コードレビューで小西さんから比べられる変数を左側に持ってきた方が読みやすいと言われ、自分は数直線上に並べたほうが読みやすいと主張し、許してもらったことがあります。

お二人ともありがとうございます！なるほどです。
(nodchip さんは小西さんと一緒に働かれてたんですね)

nodchip · 2024-08-27T09:31:13Z

arai60/Graph_BFS_DFS/number-of-islands.md

+        return numberOfIslands;
+    }
+
+    private void traverseAdjacentCells(char[][] grid, boolean[][] traversed, int m, int n, int i, int j) {


1 <= m, n <= 300 のため、最大で 90,000 回再帰する可能性があります。これによりスタックオーバーフローを引き起こす可能性があります。この点については考慮されましたか？

別コメントで対応: #17 (comment)

nodchip · 2024-08-27T09:35:03Z

arai60/Graph_BFS_DFS/number-of-islands.md

+        boolean[][] traversed = new boolean[grid.length][grid[0].length];
+        int m = grid.length;
+        int n = grid[0].length;
+        for (int i = 0; i < m; i++) {


個人的には二次元の座標を表す変数は (x, y)、(row, column)、(r, c) を使うことが多いです。ただ、最近は (i, j) も使われているようです。
https://x.com/nodchip/status/1809223275442315414
ただし、 (i, j) が横方向と縦方向のどちらを表しているかについては注意が必要です。

なるほど、個人的に意外な結果でした。

nodchip · 2024-08-27T09:38:52Z

arai60/Graph_BFS_DFS/number-of-islands.md

+
+## 再帰関数の代わりにスタックを使う
+
+(Leetcode の constraints ではそうならないとは書いてあるものの) 大きな grid が渡されたらスタックオーバーフローになる可能性があるため、スタックを用いたより安全な実装にしてみる。


Leetcode の constraints ではそうならないとは書いてある

これはどのように書かれていましたか？

すみません、こちら勘違いでした。別のコメントでも指摘頂いている通り最大90,000スタックフレームが積まれるので、スタックオーバーフローになる可能性はあると思います。
確か oda さんが以前 Java の再帰呼び出しの限界について資料を貼ってくれていたので、そちら確認の上、後ほど訂正します。

以下 oda さんの投稿からの引用です。Leetcode の constraints には "1 <= m, n <= 300" とあり、別のコメントでも指摘して頂いている通り 90,000 回再帰する可能性がありますので、スタックオーバーフローが起こる可能性があることは constraints からも明らかです。

Java は環境依存だけれども、1M くらいなので1万も行けばいいほうだよな、という感覚はあります。

from: https://discord.com/channels/1084280443945353267/1235829049511903273/1236256946403807323

誤りのあった文面を訂正しました: #17

nodchip · 2024-08-27T09:39:38Z

arai60/Graph_BFS_DFS/number-of-islands.md

+```
+
+- "This class is likely to be faster than Stack when used as a stack" とある ArrayDeque を Stack クラスの代わりに使ってみた
+    - nodachip さんも言及されてた: https://github.com/goto-untrapped/Arai60/pull/38#discussion_r1683649014


nit: nodchip

失礼しました😇（普段の呼び方に引きずられた…）

修正済✍️: ea98eb2

nodchip · 2024-08-27T09:41:03Z

arai60/Graph_BFS_DFS/number-of-islands.md

+    - しかしメモリ消費量は1~2割ほど削減される
+- 再帰関数を使った場合と比べおよそ2倍の処理時間がかかっている
+    - スタック的な役割のオブジェクトの利用よりも、スタックフレームの利用の方がよっぽど最適化されているということなのだろうか？
+    - 他に致命的な要因が無いかと record の代わりに整数型配列を使ったりもしたが、それほど大きな影響はなかった


ArrayDeque に挿入するオブジェクトのヒープメモリの確保に時間がかかっている可能性があります。

一般に new は重い処理ですので、自然に避けられるのであれば避けたほうが無難だと思います。

ありがとうございます。現在セルの数だけ new するようになっているので、避けるように工夫してみます。

ArrayDeque の宣言を numIslands で一度だけ実施し、走査の度に実行される traverseAdjacentLands に渡すように修正したところ、興味深いことにむしろ少し処理が遅くなりました (7ms -> 9ms)。
こちら修正後の実装です。

class Solution { private record Cell(int row, int column) {}; public int numIslands(char[][] grid) { int islandCount = 0; int rowCount = grid.length; int columnCount = grid[0].length; boolean[][] traversedLands = new boolean[rowCount][columnCount]; Deque<Cell> traversingCells = new ArrayDeque<>(); for (int row = 0; row < rowCount; row++) { for (int column = 0; column < columnCount; column++) { if (grid[row][column] == '0' || traversedLands[row][column] == true) { continue; } traverseAdjacentLands(grid, traversedLands, traversingCells, rowCount, columnCount, row, column); islandCount++; } } return islandCount; } private void traverseAdjacentLands(char[][] grid, boolean[][] traversedLands, Deque<Cell> traversingCells, int rowCount, int columnCount, int row, int column) { traversingCells.add(new Cell(row, column)); while (!traversingCells.isEmpty()) { Cell cell = traversingCells.removeFirst(); boolean isOutOfBounds = !(cell.row >= 0 && cell.row < rowCount && cell.column >= 0 && cell.column < columnCount); if (isOutOfBounds || grid[cell.row][cell.column] == '0' || traversedLands[cell.row][cell.column] == true) { continue; } traversedLands[cell.row][cell.column] = true; traversingCells.add(new Cell(cell.row + 1, cell.column)); traversingCells.add(new Cell(cell.row - 1, cell.column)); traversingCells.add(new Cell(cell.row, cell.column + 1)); traversingCells.add(new Cell(cell.row, cell.column - 1)); } } }

JVM が実行していた何らかの最適化がされなくなり、その影響と修正の結果生じる関数への値渡し (厳密に言うと参照値の値渡しでしょうか) のオーバーヘッドが合わさって実行時間がよりかかるようになってしまったのかな、と推測しています。
もし他に原因になってそうなものがあればご教示頂けますと嬉しいです。

ArrayDeque の宣言を numIslands で一度だけ実施し、走査の度に実行される traverseAdjacentLands に渡すように修正したところ、興味深いことにむしろ少し処理が遅くなりました (7ms -> 9ms)。

他の方がおっしゃっていたのですが、 7ms -> 9ms というのが LeetCode 上での実行時間のことであれば、計測誤差が大きいため、あまり気にしないほうが良いと思います。ただ、桁が変わってくる場合には、根本的な部分の処理不可が変わった可能性もあるため、気にしたほうが良いかもしれません。

はい、 Leetcode 上の実行時間のことです。そうですね、仕様上それなりの計測誤差が発生し得るのはしょうがないので、桁が変わるぐらいの変化で無い限りは基本的に気にしないようにしようと思います。

Yoshiki-Iwasa · 2024-08-29T05:25:48Z

arai60/Graph_BFS_DFS/number-of-islands.md

+            return;
+        }
+
+        grid[row][column] = '0';


'0'がマジックナンバー化してしまってるので名前をつけてあげたほうが良いと思います

あと、訪問済みとするなら訪問済みを表現するための他の値を使ったほうがいいかと
grid[row][column] = '0';が海に塗り替える操作なのか、訪問済みとする操作なのか読者が意図を読み取れないので

ありがとうございます。修正して Step 4 の解答に反映しました。

Yoshiki-Iwasa · 2024-08-29T05:28:58Z

arai60/Graph_BFS_DFS/number-of-islands.md

+        int islandCount = 0;
+        int rowCount = grid.length;
+        int columnCount = grid[0].length;
+        boolean[][] traversedLands = new boolean[rowCount][columnCount];


型と名前が合ってないと感じました

traversedLandsという変数名に格納されるデータは何かしらLandを表現するものであるべきかなと
この変数は訪問済みかどうかを表す真偽値を格納するのでisVisitedとかで十分なのではないでしょうか

追記
https://github.com/seal-azarashi/leetcode/pull/17/files#diff-5a321eb303c17e1955dbf9cf8de8318afdd9ff3496f0d55b36327caa0fb1682eR59

この問題も疑問形で始まる変数名なら大丈夫かなと

こちら同じ感想を持ちました。

確かにそうですね。修正して Step 4 の解答に反映しました。

Ryotaro25 · 2024-08-29T08:35:29Z

arai60/Graph_BFS_DFS/number-of-islands.md

+
+                grid[row][column] = '0';
+                if (row - 1 >= 0 && grid[row - 1][column] == '1') {
+                    unionFindIslands.union(row * columnCount + column, (row - 1) * columnCount + column);


row * columnCount + columnを使いまわされているので変数に入れてもいいかもですね。

確かにそうですね。修正しました: 0bdbe7b

Ryotaro25 · 2024-08-29T08:58:52Z

全体的に読みやすいと感じました。

goto-untrapped · 2024-08-29T13:37:26Z

arai60/Graph_BFS_DFS/number-of-islands.md

+
+    private void traverseAdjacentLands(char[][] grid, boolean[][] traversedLands, int rowCount, int columnCount, int row, int column) {
+        Deque<Cell> traversingCells = new ArrayDeque<>();
+        traversingCells.add(new Cell(row, column));


Deque, removeFirst() を使われているので、addLast() のほうがなじむ気がします。

ありがとうございます！こちらレビューで指摘してたのに自分のコードには反映出来てませんでしたね😇 Step 4 の実装に反映させました。

goto-untrapped · 2024-08-29T14:24:41Z

Java の裏側の処理など、参考になりました🙏

nittoco · 2024-09-02T13:38:05Z

arai60/Graph_BFS_DFS/number-of-islands.md

+        }
+
+        private int find(int i) {
+            if (parent[i] != i) {


細かいですが、個人的にはparent[i] == iの方を先に書きたい気持ちになりました(特殊な処理を先に書きたいので)

確かにそうした方がよりしっくりくるなとと思いました。ありがとうございます。

修正済: 6e6dd5e

nittoco · 2024-09-02T13:47:38Z

arai60/Graph_BFS_DFS/number-of-islands.md

+                if (row - 1 >= 0 && grid[row - 1][column] == '1') {
+                    unionFindIslands.union(root, (row - 1) * columnCount + column);
+                }
+                if (row + 1 < rowCount && grid[row + 1][column] == '1') {
+                    unionFindIslands.union(root, (row + 1) * columnCount + column);
+                }
+                if (column - 1 >= 0 && grid[row][column - 1] == '1') {
+                    unionFindIslands.union(root, row * columnCount + column - 1);
+                }
+                if (column + 1 < columnCount && grid[row][column + 1] == '1') {
+                    unionFindIslands.union(root, root + 1);
+                }


自分の脳内処理が間違ってたら申し訳ないのですが、これって4方向繋ぐ必要ありましたっけ(左と上だけでいいような)
また個人的には、変数名をcurrent_column(rootのやつ), up_column((row - 1) * columnCount + columnのやつ)などとつけた方がいいように思いました。
rootは多分UnionFindを意識して書いていると思うのですが、読んでいる側からすると既にそれはわかってて、知りたいのはどの位置のgridを繋げているか、だと思いました。

ありがとうございます。仰るとおり繋ぐのは2方向だけで良いですね。
root の変数名についても確かにその通りだなと思ったので、 coordinate に修正しました。
f086619

seal_azarashi added 6 commits August 23, 2024 08:48

step 1

6e22f2c

add more to step 1

6ff46d2

step 2 without reading other solutions

3b59233

fix step 1

ab389b2

step 2

648289a

step 3

6225483

nodchip reviewed Aug 27, 2024

View reviewed changes

seal_azarashi added 4 commits August 28, 2024 07:49

Step 4

6d8049d

correct

49f66ce

correct nodachip

ea98eb2

union find

04a46e8

seal-azarashi requested a review from nodchip August 28, 2024 23:17

Yoshiki-Iwasa reviewed Aug 29, 2024

View reviewed changes

Ryotaro25 reviewed Aug 29, 2024

View reviewed changes

goto-untrapped reviewed Aug 29, 2024

View reviewed changes

seal_azarashi added 4 commits September 2, 2024 07:42

iwasa san revier

d2fd19f

fix

0bdbe7b

fix

e50d011

fix

957a3ce

nittoco reviewed Sep 2, 2024

View reviewed changes

seal_azarashi added 2 commits September 3, 2024 07:43

fix

6e6dd5e

fix

f086619

haniwachann mentioned this pull request Nov 15, 2024

200.Number of islands haniwachann/leetcode#3

Open

colorbox mentioned this pull request Dec 11, 2024

200. Number of Islands colorbox/leetcode#31

Merged


		## 再帰関数の代わりにスタックを使う

		(Leetcode の constraints ではそうならないとは書いてあるものの) 大きな grid が渡されたらスタックオーバーフローになる可能性があるため、スタックを用いたより安全な実装にしてみる。

Conversation

seal-azarashi commented Aug 27, 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!

seal-azarashi Aug 27, 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!

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!

seal-azarashi Aug 27, 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!

seal-azarashi Aug 27, 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!

Yoshiki-Iwasa Aug 29, 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!

Ryotaro25 commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goto-untrapped commented Aug 29, 2024

seal-azarashi Aug 27, 2024 •

edited

Loading

seal-azarashi Aug 27, 2024 •

edited

Loading

seal-azarashi Aug 27, 2024 •

edited

Loading

Yoshiki-Iwasa Aug 29, 2024 •

edited

Loading

nittoco Sep 2, 2024 •

edited

Loading

nittoco Sep 2, 2024 •

edited

Loading