Graph bfs dfs 200#17
Conversation
jjysogfy
left a comment
jjysogfy
left a comment
There was a problem hiding this comment.
いくつかコメントさせてもらいました。何かあればお気軽にコメントいただければと思います。
| - 問題: [200. Number of Islands](https://leetcode.com/problems/number-of-islands/submissions/2008160365/) | ||
| - コメント集: [200. Number of Islands](https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/mobilebasic#h.o0jquy48e6cy) | ||
| - 一応全て確認したが、Python関係の話が多かったと思う | ||
| - コメント先のPullRequestで、column, row の変数名の話と引数を変更するのはどうなんだ？という話があったので考えてみた |
There was a problem hiding this comment.
考えてみた内容か、それを踏まえたコードがあると良いと思います。
変数名を変えた方が良いと思ったのかそうでないのか（あるいはよくわからないか）、読み取れずレビューで迷いました。
|
|
||
| while (!lands.isEmpty()) { | ||
| int[] index = lands.poll(); | ||
| int numRow = index[0]; |
There was a problem hiding this comment.
考えているかもしれませんが、この変数名は見づらく感じました。numと付くと「rowの個数」のように見えるからです。
これをrowIndexとし、引数の方はstartRowIndexとするのはどうでしょうか。
| } | ||
|
|
||
| private int numIslandsHelper(char[][] grid, int rowIndex, int columnIndex) { | ||
| Queue<int[]> lands = new ArrayDeque<>(); |
There was a problem hiding this comment.
可読性を考えるとおっしゃる通りかと。
ただ、うーーんこの規模であればint[]で十分だと思ってます
| return result; | ||
| } | ||
|
|
||
| private int numIslandsHelper(char[][] grid, int rowIndex, int columnIndex) { |
There was a problem hiding this comment.
このメソッドは、numIslandsHelperと名付けるにはnumIslandsと違いすぎると思いました。返り値も0か1だけですし。
良い名前が思いつかないですが、たとえばtraverseはどうでしょうか。
There was a problem hiding this comment.
同感です。今回は特に副作用としてgridの書き換えも行なっているのでそれがわかるようにしたいですね。
自分が見ていていいなと思ったのはsinkIslandsなどですね。1を0に置き換えることをsinkと表現するのがわかりやすいです
There was a problem hiding this comment.
お二人ともありがとうございます、
Traversal は処理の意味として理解できていいですね。
自分的にはsink がかなりしっくり来ました。
陸を沈めるってことですね、いいですね！
| } | ||
|
|
||
| while (!lands.isEmpty()) { | ||
| int[] index = lands.poll(); |
There was a problem hiding this comment.
細かいかもしれませんが、キューへの追加と削除についてadd/pollを使っていますが、add/removeかoffer/pollのどちらかだけを使う方が統一感があり好みです。
https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/util/Queue.html
ArrayDequeなのでどちらでも差はなさそうですが。
There was a problem hiding this comment.
コメントありがとうございます、読みました。
統一感、なるほど確かにそうかもしれないですね。実際私のコードでは統一されていません。
私の認識で恐縮ですが、どちらかというと、例外投げて欲しければ add/remove, 例外ではなく値が欲しいなら offer/poll を使おうということで、文脈に応じて使い分けるものかなと思っているのですが、どうでしょう？
There was a problem hiding this comment.
すみません、そうですね。間違って覚えていました……。
addとofferはArrayDequeなら同じだと思いますが（ソースコード https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/ArrayDeque.java ）、removeとpollは空の場合に違いますね。使い分けるものなのだろうと思いました。
| int numColumn = index[1]; | ||
|
|
||
| // 上下左右を探索して、1ならqueueにつめる | ||
| int nextNumRow = numRow + 1; |
| - コメント集: [](https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/mobilebasic#h.o0jquy48e6cy) | ||
| - 問題: [200. Number of Islands](https://leetcode.com/problems/number-of-islands/submissions/2008160365/) | ||
| - コメント集: [200. Number of Islands](https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/mobilebasic#h.o0jquy48e6cy) | ||
| - 一応全て確認したが、Python関係の話が多かったと思う |
There was a problem hiding this comment.
コメント集にあったか覚えてないですが、今回は与えられた配列を破壊するべきかしないべきかの検討があってもいいと思いました。
例えば、何か島を作る関数が別であった時などにデバッグで使うを想定すると、今回は引数で与えられた配列は破壊したくないですね
There was a problem hiding this comment.
ありがとうございます。
ここ、対応しようと思って忘れてました。
取り込みます。
| int result = 0; | ||
| for (int rowIndex = 0; rowIndex < grid.length; rowIndex++) { | ||
| for (int columnIndex = 0; columnIndex < grid[0].length; columnIndex++) { | ||
| result += numIslandsHelper(grid, rowIndex, columnIndex); |
There was a problem hiding this comment.
この関数に返り値はなくてもいいかなと思いました。島があるかないかで1か0が返ってくるだけなので。
There was a problem hiding this comment.
おっしゃる通りですね。
別に += する必要がないですね。。
| ```java | ||
| class Solution { | ||
| public int numIslands(char[][] grid) { | ||
| if (grid.length == 0) return 0; |
There was a problem hiding this comment.
こちらのコメントをご参照ください。
Shunii85/arai60#17 (comment)
|
|
||
| private int numIslandsHelper(char[][] grid, int rowIndex, int columnIndex) { | ||
| // index ouf of range | ||
| if ((rowIndex < 0 || columnIndex < 0) || (rowIndex >= grid.length || columnIndex >= grid[0].length)) { |
There was a problem hiding this comment.
数直線上に一直線上に並ぶように書いたほうが、読み手にとって読みやすくなると思います。
if (!(0 <= rowIndex && rowIndex < grid.length && 0 <= columnIndex && columnIndex < grid[0].length)) {| } | ||
| // find land | ||
| if (grid[rowIndex][columnIndex] == '1') { | ||
| grid[rowIndex][columnIndex] = '0'; |
There was a problem hiding this comment.
こちらのコメントをご参照ください。
Hiroto-Iizuka/coding_practice#17 (comment)
| private int numIslandsHelper(char[][] grid, int rowIndex, int columnIndex) { | ||
| Queue<int[]> lands = new ArrayDeque<>(); | ||
| // find land | ||
| if (grid[rowIndex][columnIndex] == '1') { |
There was a problem hiding this comment.
もう少しシンプルに early return したほうが、読み手にとって読みやすくなると思います。
if (grid[rowIndex][columnIndex] == '0') {
return 0;
}
grid[rowIndex][columnIndex] = '0';
lands.add(new int[]{rowIndex, columnIndex});5 participants
200. Number of Islandsを解きました。
レビューお願いします。