200. Number of Islands #17

garunitule · 2025-06-26T13:40:59Z

問題：200. Number of Islands 200. Number of Islands
次の問題：695. Max Area of Island 695. Max Area of Island

huyfififi · 2025-06-26T20:14:58Z

Graph/200/memo.md

+```
+
+## 他の人のPRやコメントを踏まえて実装
+leetcodeの解法を見ると、dfs, bfs, union findの3つあった。union findは名前しか知らなかったので調査する。


いわゆる競プロの問題集(e.g., Algorithms for Competitive Programming)でUnion Findに何回か遭遇した記憶がありますが、一般常識ではないような気がする（？）ので、インタビュー時は素直にBFSかDFSを実装するのが無難かもしれませんね。レビュワーが一回調べる手間が入るかも 🤔

個人的な感覚、union find は現実的には多くの人が知っているし、知らなくても書かれれば読めるんですが、しかし、知らない同僚がいたとしても別に動揺しないというラインだと思いますね。

一方で、書いてきたらきれいに書かれているかは気にするでしょうね。クラスの設計が評価に入るので、難易度は高めのコースを自分で選んだようには見えるでしょう。

なるほど、UnionFindは必須ではないレベルの知識なんですね

huyfififi · 2025-06-26T20:21:49Z

Graph/200/memo.md

+        n = len(grid[0])
+        visited = [[False for _ in range(n)] for _ in range(m)]
+
+        def visit_adjacent_lands(grid: List[List[str]], i: int, j: int) -> List[List[str]]:


ごく個人的に、私だったら返り値はNoneにしますね。その方が、この関数は副作用がある (visitedを書き換えている)とシグネチャを見るだけでわかりやすいので。

参考: Martin Fowler's explanation on Command Query Separation

今回はgridを変更していないので混乱させてしまいますね、ありがとうございます！

huyfififi · 2025-06-26T20:28:11Z

Graph/200/memo.md

+        def visit_adjacent_lands(grid: List[List[str]], i: int, j: int) -> List[List[str]]:
+            visited[i][j] = True
+
+            if 0 <= i - 1 and grid[i - 1][j] == '1' and not visited[i - 1][j]:


確かに、先に範囲・visitedチェックをしておくとstackの使用が多少省けるのですが、私は後でチェックする方が読みやすく感じます。今回の場合、大きくパフォーマンスにも影響しないと思いますし、可読性を優先しても良いかな、と。

def visit_adjacent_lands(row: int, col: int) -> None: if not (0 <= row < num_rows) or not (0 <= col < num_cols): return if visited[row][col]: return if grid[row][col] == "0": return

書いてみたのですが、たしかに可読性高いですね。
条件式を4回書いてたのがなくなってスッキリしました

visit_adjacent_landsの返り値はboolにしてみました
Noneだと、for文の中でvisit_adjacent_landsの呼び出しとnum_islandsのインクリメントにif文が必要になって条件式が重複するためです

visit_adjacent_landsの返り値がNoneの場合のコード

class Solution: def numIslands(self, grid: List[List[str]]) -> int: num_rows = len(grid) num_cols = len(grid[0]) visited = [[False] * num_cols for _ in range(num_rows)] WATER = '0' LAND = '1' num_islands = 0 def visit_adjacent_lands(row: int, col: int) -> None: if not (0 <= row < num_rows and 0 <= col < num_cols): return if grid[row][col] == WATER: return if visited[row][col]: return visited[row][col] = True visit_adjacent_lands(row - 1, col) visit_adjacent_lands(row, col - 1) visit_adjacent_lands(row + 1, col) visit_adjacent_lands(row, col + 1) for row in range(num_rows): for col in range(num_cols): if grid[row][col] == LAND and not visited[row][col]: visit_adjacent_lands(row, col) num_islands += 1 return num_islands

visit_adjacent_landsの返り値がboolの場合のコード

class Solution: def numIslands(self, grid: List[List[str]]) -> int: num_rows = len(grid) num_cols = len(grid[0]) visited = [[False] * num_cols for _ in range(num_rows)] WATER = '0' LAND = '1' num_islands = 0 def visit_adjacent_lands(row: int, col: int) -> bool: if not (0 <= row < num_rows and 0 <= col < num_cols): return False if grid[row][col] == WATER: return False if visited[row][col]: return False visited[row][col] = True visit_adjacent_lands(row - 1, col) visit_adjacent_lands(row, col - 1) visit_adjacent_lands(row + 1, col) visit_adjacent_lands(row, col + 1) return True for row in range(num_rows): for col in range(num_cols): if visit_adjacent_lands(row, col): num_islands += 1 return num_islands

huyfififi · 2025-06-26T20:31:17Z

Graph/200/memo.md

+```python
+class Solution:
+    def numIslands(self, grid: List[List[str]]) -> int:
+        m = len(grid)


m, nが matrixのrow, columnの長さを表す、ってどれくらい一般的なのでしょうかね。私はnum_rows, num_colsの方がわかりやすく感じますが...

num_rows, num_colsのほうが分かりやすいですね、ありがとうございます
自分の感覚だとよくある気がしていましたが、実務だとあまり見たことがないと思いました

huyfififi · 2025-06-26T20:33:02Z

Graph/200/memo.md

+                grid = visit_adjacent_lands(grid, i, j + 1)
+            return grid
+
+        numberOfislands = 0


LeetCodeに合わせているものだと思いますが、一応、Pythonでの変数名はsnake_caseの方が一般的かなと思います。
https://peps.python.org/pep-0008/#function-and-variable-names

huyfififi · 2025-06-26T20:34:30Z

Graph/200/memo.md

+    def numIslands(self, grid: List[List[str]]) -> int:
+        m = len(grid)
+        n = len(grid[0])
+        visited = [[False for _ in range(n)] for _ in range(m)]


私は [[False] * n for _ in range(m)]と書きますが、どっちもどっちな気がします

外側のループが内包表記なのが気になったのですが、下記のような理由があったのですね
https://docs.python.org/3/faq/programming.html#how-do-i-create-a-multidimensional-list

勉強になりました！

huyfififi · 2025-06-26T20:36:50Z

Graph/200/memo.md

+
+# step2: 30分
+## 自分でコード読みやすく整えて実装
+メソッド名をdfsからvisit_adjacent_landsに修正した


良いと思います 👍 dfs()だと、なんの操作をするのかわかりづらいですよね、ユーザからしたら、方法はなんでも良いから何をするのか教えてくれ、となりそうな気がします。

huyfififi · 2025-06-26T20:38:50Z

Graph/200/memo.md

+LAND = '1'
+
+class UnionFind:
+    def __init__(self, grid: List[List[str]]):


ちなみに、最近のPythonのバージョンではlist[list[str]]の方が推奨のようですね
https://docs.python.org/3/library/typing.html#typing.List

huyfififi · 2025-06-26T20:40:46Z

Graph/200/memo.md

+空間計算量：O(m * n)
+
+## 実装
+最初 `dfs`の中で島かどうか判定する処理を抜かしてしまっていた


これ、私も同じことをして最初のDFSでvisitedが全てTrueになりました 😂

huyfififi · 2025-06-26T23:21:22Z

Graph/200/memo.md

+
+        numberOfislands = 0
+        for i in range(m):
+            for j in range(n):


i, jが何を表しているのかわかりづらいので、row_i, col_iなどとするのはいかがでしょうか。
確かThe Art of Readable Code内で、わかりやすいloop iteratorsの命名について言及されていたような気がするので、確認してみていただくのも良いかもしれません。

特に複数のループがある場合はちゃんと分かる名前にした方が良さそうですね
リーダブルコードも確認してみます

nodchip · 2025-06-27T09:33:29Z

Graph/200/memo.md

+        n = len(grid[0])
+        visited = [[False for _ in range(n)] for _ in range(m)]
+
+        def dfs(grid: List[List[str]], i: int, j: int) -> List[List[str]]:


あくまで個人的な好みですが、縦横の座標を表す変数名は (x, y)、(row, column)、(r, c) などを使います。 (i, j) は行列の添え字には使いますが、縦横の座標を表す際にはあまり使わないです。

nodchip · 2025-06-27T09:35:40Z

Graph/200/memo.md

+
+        for i in range(m):
+            for j in range(n):
+                if grid[i][j] == '1':


if grid[i][j] == '0': continue

としたほうが、ネストが浅くなり、読みやすくなると思います。

nodchip · 2025-06-27T09:37:35Z

Graph/200/memo.md

+unionの処理が間違っていた。
+```python
+class UnionFind:
+    def __init__(self, grid: List[List[str]]):


UnionFind のクラスが、外側の LeetCode の問題の内容を知らなければならないという設計に和漢を感じました。自分なら UnionFind の要素のサイズ (m * n) のみコンストラクターで渡すと思います。

たしかに、コンストラクタの内部ロジックもgridに依存しているので分かりづらいですね
UnionFind自体はこの問題とは独立して実装しておくべきでした（その方が再利用もできる）

nodchip · 2025-06-27T09:38:01Z

Graph/200/memo.md

+    def __init__(self, grid: List[List[str]]):
+        m = len(grid)
+        n = len(grid[0])
+        self.parent = {}


list で実装したほうが処理が軽くなると思いました。

tokuhirat · 2025-06-27T14:38:17Z

Graph/200/memo.md

+            visited[i][j] = True
+
+            if 0 <= i - 1 and grid[i - 1][j] == '1' and not visited[i - 1][j]:
+                grid = dfs(grid, i - 1, j)


ここを読むと、grid が更新されるのかなと思いました。更新しない、引数に入れなくても grid にはアクセスできるので、引数には grid は不要と思いました。

たしかに、numIslands内でdfsを定義しているので不要ですね

ryosuketc

下書きで置いておいたら他の方のレビューも入ってました。内容は重複しているものがほとんどのようです。一応レビューしましたということで submit しておきます。全体的に変数名 (関数も含む) の付け方に改善の余地があると思います。まだであればこのあたりもご覧になるとよいかもしれません。
https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0#heading=h.fcs3httrll4l

ryosuketc · 2025-06-27T21:51:13Z

Graph/200/memo.md

+        m = len(grid)
+        n = len(grid[0])


個人的にはm, nどっちがどっちがわからなくなるので rows, cols とかの方が好きです。

ryosuketc · 2025-06-27T21:54:14Z

Graph/200/memo.md

+    def numIslands(self, grid: List[List[str]]) -> int:
+        m = len(grid)
+        n = len(grid[0])
+        visited = [[False for _ in range(n)] for _ in range(m)]


visited = [[False] * rows for _ in range(cols)]

とも書けますね。好みだと思います。

ryosuketc · 2025-06-27T21:55:35Z

Graph/200/memo.md

+        n = len(grid[0])
+        visited = [[False for _ in range(n)] for _ in range(m)]
+
+        def dfs(grid: List[List[str]], i: int, j: int) -> List[List[str]]:


個人的に、dfs という関数名はあまり意味がないので好きではないです。traverse_island とかもうちょっと意味のあるものにしたい感覚があります。

garunitule · 2025-06-30T03:07:49Z

ありがとうございます！
一読しておきます！

200: step1, 2, 3

03ea744

huyfififi approved these changes Jun 26, 2025

View reviewed changes

huyfififi reviewed Jun 26, 2025

View reviewed changes

nodchip reviewed Jun 27, 2025

View reviewed changes

tokuhirat reviewed Jun 27, 2025

View reviewed changes

garunitule merged commit d0b8401 into main Jun 30, 2025

ryosuketc reviewed Jun 30, 2025

View reviewed changes

200. Number of Islands #17

200. Number of Islands #17

Uh oh!

Conversation

garunitule commented Jun 26, 2025

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!

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!

ryosuketc left a comment

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!

garunitule commented Jun 30, 2025

Uh oh!

Uh oh!