200. Number of Islands#17
Conversation
| 1st | ||
| 思いついたのが島を見つけたら島を再帰的に訪問済みにしその後カウントする方法。 | ||
| 再帰的な DFS。 | ||
| 今回の制約上、m,n は最大で 300 なので、再帰の深さは最大 90,000 になる可能性がある。 |
There was a problem hiding this comment.
上、下、右、左の順序で再帰していくので、確かにこれは場面全部を蛇腹状に埋める形で再帰が深くなっていきますね。
あー、順番変えても一緒ですか。
| rows = len(grid) | ||
| cols = len(grid[0]) |
There was a problem hiding this comment.
rows や cols といった変数名の場合、row という object のリストのように誤解を招きそうです。どこまで丁寧に書くかは難しいですが、下で書いている number_of_islands と同様に number_of_rows や短縮形で num_rows 、もしくは問題文中で与えられている m あたりが候補に挙がりそうです。
There was a problem hiding this comment.
省略せずにnumber_of_rowsなどのほうがより認知負荷が低く読みやすいですね。
ありがとうございます。
|
|
||
| def visit_islands(row, col): | ||
| if not (0 <= row < rows and 0 <= col < cols and grid[row][col] == "1"): | ||
| return None |
There was a problem hiding this comment.
return None と返り値を指定しない return はどちらも None を返すので結果は同じになりますが、明示的に None と書くとそこに意味があるのかなと読み手が感じそうです。ここでの return は単に関数の処理を終了させたい意図かと思うので、シンプルに return とするのがよさそうです。（逆に値があればそれを返し、なければ None を返すといった処理をする関数の場合は return None が適切になりそうです）
There was a problem hiding this comment.
おっしゃるとおり、Noneを省略してもNoneを返すので、明示的に書いたほうがわかりやすいかなと思ったのですが、PEP8にも
Be consistent in return statements.
If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None
とあったので、他のreturn文で値を帰す場合はNoneを明示的にし、関数を抜けるためだけのreturnは単にreturnだけでよさそうです。
ありがとうございます。
| def visit_islands(row, col): | ||
| if not (0 <= row < rows and 0 <= col < cols and grid[row][col] == "1"): | ||
| return None | ||
| grid[row][col] = "visited" |
There was a problem hiding this comment.
与えられた grid に破壊的な変更を加えていることは認識できていますかね？破壊的な変更は望ましくないケースが多いと思うので、基本的に避けたほうがよいと思います。grid とは別で訪問済みかどうかを管理すると破壊的な変更が不要になります。
There was a problem hiding this comment.
はい。元の2次元配列を破壊していることは認識していました。
ただ今回、入力の値を変更してはならないという制約がなかったため、破壊的な方法を採用しました。
もし面接や仕事で非破壊的に書いてくださいと言われたら、その指示には従います。
今回、その旨をコメントなどに書いていなかったので、そういったこともコメントに書くようにします。
There was a problem hiding this comment.
pythonのリストは値渡しだと思うので破壊的変更ではないと思ったのですが、そういうわけではないのですか？
There was a problem hiding this comment.
制約がなかったため
これについては、(結論はどちらでもいいのですが基本的には)関数を呼ぶ人の気持ちになるといいと思いますね。
それと、この関数を呼んでいるコードを読んでいる人は、おそらく仕様を知らずに読んでいるので、その人達を驚かせないようにしたいです。
値渡し
Python は、変数名の空間とオブジェクトの空間があります。https://discord.com/channels/1084280443945353267/1226508154833993788/1227277164169265172
setitem が出てくるあたりまでを見ておいてください。
There was a problem hiding this comment.
setitem が出てくるあたりまでを見ておいてください。
読みました。x = 0 というコードが「xの指す先の値を0にする」ではなく、「0というオブジェクトをxに束縛(bind)する」という読み方をする、ということを初めて知りました
| for x, y in [(1,0), (-1, 0), (0,1), (0, -1)]: | ||
| visit_islands(row + x, col + y) |
There was a problem hiding this comment.
細かい話ですが、x, y からは x 軸 y 軸を想像しますが、ここでは y 軸方向に + x をし、x 軸方向に + y をしているので違和感を覚えました。その部分で読み手が混乱する可能性があるので、2次元配列の場合は基本的には x, y は避けたほうがよいかもです。（grid[y][x] とすると軸の観点では一致しますが、アルファベットでは逆順なのでそれはそれで違和感があるという）
There was a problem hiding this comment.
step2 のように、 x, y の代わりに delta_row, delta_col としたほうが分かりやすいと感じました。
| stack = [] | ||
| stack.append([start_row, start_col]) |
There was a problem hiding this comment.
stack はデータ構造自体を意味するので、中身の情報も変数名に含めるとよさそうです。
また細かいですが、初期化時に [start_row, start_col] を入れておくと1行で書けます。
There was a problem hiding this comment.
そうですね。stackは抽象的なので、中身がわかりやすい命名を考えてみます。
| stack = [] | ||
| stack.append([start_row, start_col]) | ||
| while stack: | ||
| current_row, current_col = stack.pop() |
There was a problem hiding this comment.
current_row の current にはあまり意味がないので、シンプルに row でもよいかもです。
| for x, y in [(1,0), (-1, 0), (0,1), (0, -1)]: | ||
| visit_islands(row + x, col + y) |
There was a problem hiding this comment.
step2 のように、 x, y の代わりに delta_row, delta_col としたほうが分かりやすいと感じました。
| if not (0 <= row < rows and 0 <= col < cols and grid[row][col] == "1"): | ||
| return None | ||
| grid[row][col] = "visited" | ||
| for x, y in [(1,0), (-1, 0), (0,1), (0, -1)]: |
There was a problem hiding this comment.
, のあとにスペースを空けることをおすすめいたします。
(0,1) → (0, 1)
| visit_islands(row + x, col + y) | ||
|
|
||
|
|
||
| for i in range(rows): |
There was a problem hiding this comment.
step2 のように row/col としたほうが理解しやすいように感じました。
| number_of_islands = 0 | ||
|
|
||
| def visit_islands(row, col): | ||
| if not (0 <= row < rows and 0 <= col < cols and grid[row][col] == "1"): |
There was a problem hiding this comment.
個人的には、無駄な関数の呼び出しを省くため、関数の再帰呼び出しをする直前にこの判定を入れたほうがよいと思います。
| uf = UnionFind(nodes) | ||
| directions = [(0,1), (1,0), (-1,0), (0,-1)] | ||
| for r, c in nodes: | ||
| for dr, dc in directions: |
There was a problem hiding this comment.
delta_row, delta_col の略だと思いますが、 dr dc とまで略すると、何を表しているのか分からない人が多くなるだろうと思いました。delta_r, delta_c もしくは row, col, delta_row, delta_col と書いたほうがよいと思います。
| directions = [(0,1), (1,0), (-1,0), (0,-1)] | ||
| for r, c in nodes: | ||
| for dr, dc in directions: | ||
| nr, nc = r + dr, c + dc |
There was a problem hiding this comment.
nr, nc が何の略なのか分かりませんでした。 next_row, neighbor_row あたりでしょうか…。フルスペルで書くことをおすすめいたします。
| nr, nc = r + dr, c + dc | ||
| if (nr, nc) in uf.parent: | ||
| uf.union((r, c), (nr, nc)) | ||
| root_set = set() |
There was a problem hiding this comment.
島の数をカウントするのに set を使うのは、大げさなように感じました。
num_islands = 0
for node in nodes:
if uf.find(node) == root:
num_islands += 1
return num_islands
あたりはいかがでしょうか？
There was a problem hiding this comment.
ありがとうございます。
こちらのほうがよさそうですね。
There was a problem hiding this comment.
あとは、最初のLANDの数を覚えておき、Unionした回数を引くことでも求まりますね。
| if grid[i][j] == '1': | ||
| nodes.append((i, j)) | ||
| uf = UnionFind(nodes) | ||
| directions = [(0,1), (1,0), (-1,0), (0,-1)] |
There was a problem hiding this comment.
union-findの場合は(0,1), (1,0)のチェックだけで十分だと思います。以下のような場合に(-1,0), (0,-1)がないとDFSだと正しくカウントできません。一方union-findの場合は、(0,2)のlandが(1,2)とくっつき、のちに(1,2)が(1,1)とくっつく、といった具合にちゃんとカウントできます
1 0 1 0
1 1 1 0
6 participants
https://leetcode.com/problems/number-of-islands/description/