Conversation
dxxsxsxkx
left a comment
There was a problem hiding this comment.
いろいろなパターンがconciseにまとめられていて、良いと思いました。
| return 0 | ||
| if root.right is None and root.left is None: | ||
| return 1 | ||
| min_depth = float("inf") |
There was a problem hiding this comment.
intのinfがないので、floatのinfで代用しました
(質問の意図に合っているか分かりませんが)
| - sol1.py: トップダウン + ループ(BFS) | ||
| - sol2.py: ボトムアップ + 再帰(DFS) | ||
| - sol3.py: トップダウン + 再帰(DFS) | ||
| - sol4.py: ボトムアップ + ループ(DFS / スタック) |
There was a problem hiding this comment.
結局、実務で「実装してくれ」と言われたときに、どんな理由でどれを選ぶのか、が検討できていると良いと思います。
111/sol1.py
Outdated
| def minDepth(self, root: Optional[TreeNode]) -> int: | ||
| if root is None: | ||
| return 0 | ||
| que = deque() |
There was a problem hiding this comment.
中途半端な省略は避けたほうが無難です。この練習会では frontier や、depth_and_nodes (入っているもの)などの命名をよく見かけます。
| que = deque() | |
| queue = deque() |
111/sol1.py
Outdated
| depth, node = que.popleft() | ||
| if node.right is None and node.left is None: | ||
| return depth | ||
| for child in [node.right, node.left]: |
There was a problem hiding this comment.
特に理由がなければ、left -> right の順が自然に感じました。事情があるならコメントしておくと良いでしょう。
There was a problem hiding this comment.
あと、[node.right, node.left] の配列が都度生成されることは意識しておくと良いと思います(オーバーヘッドとしては大したものではないでしょうが)。
There was a problem hiding this comment.
順序を変えてtupleにしました。たしかに配列生成のオーバーヘッドが発生しますね
111/sol3.py
Outdated
| min_depth = float("inf") | ||
| if root is None: | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
特別なケースは先に返してしまって、その後になって必要な変数を宣言したいです。
| min_depth = float("inf") | |
| if root is None: | |
| return 0 | |
| if root is None: | |
| return 0 | |
| min_depth = float("inf") |
111/sol3.py
Outdated
| if node is None: | ||
| return |
There was a problem hiding this comment.
child のNoneチェックは済んでいる、かつ root のNoneチェックは済んでいるので、ここではチェック不要そうです。
| if node is None: | |
| return |
111/sol3.py
Outdated
| if child is None: | ||
| continue | ||
| dfs(child, depth + 1) | ||
| return |
There was a problem hiding this comment.
たしかに不要ではありますね。省略するのが普通なんでしょうか。一応削除しようと思います。
There was a problem hiding this comment.
「一応」が気になったのでコメントします。削除すべきかどうかの基準・価値観を自分なりに持つことと、普通=一般的にはどうなのかを知ることが、この練習会での目的のひとつに思います(ただし、「普通」には幅があることに注意してください。実務上は所属チームに合わせるのが良いと思います)。これを踏まえて、何を考えてそうしたのか言語化してみてはどうでしょうか。
末尾に到達したら関数がreturn (None) するのはPythonの仕様で、私は、ここはそれに該当するので、特別な事情がない限りは不要(むしろ読み手が「何か事情があるのか?」と無駄に勘ぐってしまう)と考えました。一方で、「ここまで関数を抜けるときはreturnしてきたので、最後もそうするべきだ(一貫性がある)」という主張も、理屈の上では成り立つように思います。
There was a problem hiding this comment.
また、「普通」のサンプルとして、この練習会ではPEP8やGoogleのスタイルガイドが取り上げられることが多いです。これらの中も探してみると良いでしょう。ちなみに私がざっと調べたところ、「末尾のreturnは書くな」という文は見つけられませんでした。
There was a problem hiding this comment.
おっしゃるとおりで、PEP8 には明示的に書くなとは書いてないですね。ただ、値を返す関数のときには末尾に return None と書けとありますね。
ただ、Pylint に useless-return という項目があったりと、警告する linter は多そうです。
https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/useless-return.html
https://peps.python.org/pep-0008/#programming-recommendations:~:text=Be%20consistent%20in%20return%20statements.
There was a problem hiding this comment.
Pythonではreturn文を書かなくてもNoneを返す仕様になっているので、必要のないreturnは書かないようにしようと思います。
曖昧な点を残さずに課題に取り組むことも意識しようと思います。
(GoogleのStyleガイドも見てみましたが、言及されている箇所は見つかりませんでした)
https://google.github.io/styleguide/pyguide.html
111/sol3.py
Outdated
| if root is None: | ||
| return 0 | ||
|
|
||
| def dfs(node, depth): |
There was a problem hiding this comment.
dfs には読み手にとっての情報が少ないので、何か読解の手がかりになる情報を乗せてあげたいです。
111/sol4.py
Outdated
| que = deque() | ||
| que.append((root, False)) |
There was a problem hiding this comment.
末尾からpopしているのでstackで良さそうですね。
| que = deque() | |
| que.append((root, False)) | |
| que = [(root, False)] |
There was a problem hiding this comment.
stackとして用いる場合には配列(List)を用いるようにしようと思います。
111/sol4.py
Outdated
| class Solution: | ||
| def minDepth(self, root: Optional[TreeNode]) -> int: | ||
| node_to_depth = defaultdict(int) | ||
| visited_nodes = set() |
There was a problem hiding this comment.
キューに is_visited を入れているので、このsetは使っていませんね。一方で、このsetで管理したほうが分かりやすいと思いました。
There was a problem hiding this comment.
今回はis_visitedを削除しておきました。
たしかに set管理の方がわかりやすいかもしれません。
111/sol4.py
Outdated
| if is_visited: | ||
| depth_l = node_to_depth.get(node.left, 0) | ||
| depth_r = node_to_depth.get(node.right, 0) | ||
| if depth_l == 0 or depth_r == 0: | ||
| node_to_depth[node] = max(depth_l, depth_r) + 1 | ||
| else: | ||
| node_to_depth[node] = min(depth_l, depth_r) + 1 | ||
| else: | ||
| que.append((node, True)) | ||
| que.append((node.left, False)) | ||
| que.append((node.right, False)) |
There was a problem hiding this comment.
- 読むうえではnot visitedの状態から始まるので、先にその場合で分岐してearly continueするのが分かりやすいです
node_to_depthにキーが無い=その子ノードが存在しない場合に0を使っているのですが、これをmaxやminと組み合わせているのがpuzzlingです。行数は多くなりますがNoneの方が分かりやすいです
| if is_visited: | |
| depth_l = node_to_depth.get(node.left, 0) | |
| depth_r = node_to_depth.get(node.right, 0) | |
| if depth_l == 0 or depth_r == 0: | |
| node_to_depth[node] = max(depth_l, depth_r) + 1 | |
| else: | |
| node_to_depth[node] = min(depth_l, depth_r) + 1 | |
| else: | |
| que.append((node, True)) | |
| que.append((node.left, False)) | |
| que.append((node.right, False)) | |
| if not is_visited: | |
| que.append((node, True)) | |
| que.append((node.left, False)) | |
| que.append((node.right, False)) | |
| continue | |
| depth_l = node_to_depth.get(node.left, None) | |
| depth_r = node_to_depth.get(node.right, None) | |
| if depth_l is None and depth_r is None: | |
| node_to_depth[node] = 1 | |
| elif depth_l is not None and depth_r is None: | |
| node_to_depth[node] = depth_l + 1 | |
| elif depth_l is None and depth_r is not None: | |
| node_to_depth[node] = depth_r + 1 | |
| else: | |
| node_to_depth[node] = min(depth_l, depth_r) + 1 |
ついでにいうと、スタックによって子ノードから先に処理される(node_to_depthに追加される)前提が暗黙にあるのが、読解の上でやや難しいと感じました。
There was a problem hiding this comment.
読むうえではnot visitedの状態から始まるので早期 returnすること、depthのロジックが読み手に考えさせるので、もっと簡潔に書くこと、といご指摘採用しました。
自分で気づけるようになることを目指したいです。
https://leetcode.com/problems/minimum-depth-of-binary-tree/description/