2. Add Two Numbers - #5
Conversation
| if node is None: | ||
| return 0 | ||
| return node.val | ||
| def get_next_node(node): |
There was a problem hiding this comment.
個人的には inner function の後には空行を入れ、区切りであることを視覚的に表現したほうが良いと思います。チームの平均的な書き方に合わせることをおすすめします。
There was a problem hiding this comment.
個人的には inner function の後には空行を入れ、区切りであることを視覚的に表現したほうが良い
ありがとうございます！確かにその方が見やすいですね。
空行 == 心の迷いというコメントを拝見したので、意識的に空行をなくしていたのですが、inner functionの後に関しては区切りを入れた方が個人的には見やすく思いました。
naoto-iwase
left a comment
naoto-iwase
left a comment
There was a problem hiding this comment.
お疲れさまです。
Step 2で広い観点で見直しされていて素晴らしいと思います。
いくつかコメントさせていただきました。
| while l1 is not None or l2 is not None or carry != 0: | ||
| total = get_node_val(l1) + get_node_val(l2) + carry | ||
| carry = total // 10 | ||
| node.next = ListNode(total % 10) | ||
| node = node.next | ||
|
|
||
| if l1 is not None: | ||
| l1 = l1.next | ||
| if l2 is not None: | ||
| l2 = l2.next |
There was a problem hiding this comment.
if l1 is not Noneのときにやる処理が
- .valをtotalへ加算することと
- .nextへ進めること
の2つだと思いますが、これをまとめて書くこともできます。
while l1 is not None or l2 is not None or carry != 0:
total = carry
if l1:
total += l1.val
l1 = l1.next
if l2:
total += l2.val
l2 = l2.next
carry = total // 10
node.next = ListNode(total % 10)
node = node.nextThere was a problem hiding this comment.
ありがとうございます！追加しました！
if 文の数を減らせるし、get_node_val を定義しなくて良くなるのは良いなと思いました 👍
https://github.com/kazizi55/coding-challenges/pull/5/files#diff-18e3fdef3e11b8b8080b8668c810dfb10048c12f58759c0f545ba1dbbd2b1ea4R76-R94
| - Number の List を作って None を事前に取り除く (SolutionUsingNumberList) | ||
| - None チェックを減らせるのでコード量を減らせる | ||
| - 参考にした回答との差分 | ||
| - 番兵として head を用いているので、 sentinel にリネームした |
There was a problem hiding this comment.
番兵を使わず、代わりにwhileループ内に条件分岐を作る方法もあります。
head = tail = None
while l1 or l2 or carry:
...
if head is None:
head = tail = node
else:
tail.next = node
tail = tail.next
return headThere was a problem hiding this comment.
ありがとうございます！ 😄
番兵を作らない分、head と tail の両方を while ループ内で参照する必要があるのですね。
追加しました！
https://github.com/kazizi55/coding-challenges/pull/5/files#diff-18e3fdef3e11b8b8080b8668c810dfb10048c12f58759c0f545ba1dbbd2b1ea4R25-R51
| return 0 | ||
| return node.val | ||
|
|
||
| sentinel = ListNode |
There was a problem hiding this comment.
カッコ忘れでした、、
追記しました！
ちなみにカッコなしでも LeetCode 上だと通ってしまっていたのが恐ろしいですね。(ヒープ領域ではなく、データ領域のクラス変数を書き換えるので、スレッドなどで同時にこの処理を呼び出すと壊れる)
| carry = total // 10 | ||
| node.next = ListNode(total % 10) |
There was a problem hiding this comment.
divmod関数を使えば商と剰余をタプルで得られます。使うかどうかは趣味の範囲だと思います。
There was a problem hiding this comment.
ありがとうございます！
そのような便利なメソッドがあるんですね、知らなかったです。商と剰余の取得を記述量少なく書けますね。
追加しました！
https://github.com/kazizi55/coding-challenges/pull/5/files#diff-18e3fdef3e11b8b8080b8668c810dfb10048c12f58759c0f545ba1dbbd2b1ea4R25-R51
| def addTwoNumbers(self, l1: Optional[ListNode], l2: Optional[ListNode]) -> Optional[ListNode]: | ||
| def add_two_numbers(nodes: list[ListNode]): | ||
| sentinel = ListNode() | ||
| current = sentinel | ||
| carry = 0 | ||
|
|
||
| while True: | ||
| nodes = [node for node in nodes if node is not None] | ||
| if not nodes and carry == 0: | ||
| return sentinel.next | ||
| total = carry | ||
| for i in range(len(nodes)): | ||
| node = nodes[i] | ||
| total += node.val | ||
| nodes[i] = node.next | ||
| current.next = ListNode(total % 10) | ||
| current = current.next | ||
| carry = total // 10 | ||
| return add_two_numbers([l1, l2]) |
There was a problem hiding this comment.
以下の点が個人的に気になりました。
- inner functionが外側と同じ名前である点
- While TrueとしているがTrueの代わりに条件式をここに書いた方が明示的だと感じた
| def addTwoNumbers(self, l1: Optional[ListNode], l2: Optional[ListNode]) -> Optional[ListNode]: | |
| def add_two_numbers(nodes: list[ListNode]): | |
| sentinel = ListNode() | |
| current = sentinel | |
| carry = 0 | |
| while True: | |
| nodes = [node for node in nodes if node is not None] | |
| if not nodes and carry == 0: | |
| return sentinel.next | |
| total = carry | |
| for i in range(len(nodes)): | |
| node = nodes[i] | |
| total += node.val | |
| nodes[i] = node.next | |
| current.next = ListNode(total % 10) | |
| current = current.next | |
| carry = total // 10 | |
| return add_two_numbers([l1, l2]) | |
| def addTwoNumbers(self, l1: Optional[ListNode], l2: Optional[ListNode]) -> Optional[ListNode]: | |
| sentinel = ListNode() | |
| current = sentinel | |
| carry = 0 | |
| nodes = [l1, l2] | |
| nodes = [node for node in nodes if node is not None] | |
| while nodes or carry != 0: | |
| total = carry | |
| for i in range(len(nodes)): | |
| node = nodes[i] | |
| total += node.val | |
| nodes[i] = node.next | |
| current.next = ListNode(total % 10) | |
| current = current.next | |
| carry = total // 10 | |
| nodes = [node for node in nodes if node is not None] | |
| return sentinel.next |
There was a problem hiding this comment.
ありがとうございます！
以下の形で修正してみました
- 通常の loop で条件を明示して、読み手の負荷を下げる
- inner function の prefix として
_をつけることで外側の関数と同じ名前になることを避ける
| current = sentinel | ||
| carry = 0 | ||
|
|
||
| while True: |
There was a problem hiding this comment.
無限ループで書いているのにbreakが出てこないので若干困惑しました。通常のループで条件を明示したほうが読み手の負荷が低いと思います。
There was a problem hiding this comment.
ありがとうございます！
確かに無限ループだとどうやったらループが終わるかを読み手に考えさせることになるので、明示的な break を使うか、通常のループを使う方が読み手の負荷は下がりますね。
while nodes or carry != 0 で書き直しました！
https://github.com/kazizi55/coding-challenges/pull/5/files#diff-18e3fdef3e11b8b8080b8668c810dfb10048c12f58759c0f545ba1dbbd2b1ea4R53-R74
There was a problem hiding this comment.
nodes から None であるやつをフィルターするのがループの先頭だと無限ループで書きたくなりますね。
while のところに「主人公が書いてある」ほうが読みやすいのも確かとは思います。(軽い違いですが。)
フィルター機能をループの最後に回すのも一つかと思いました。(が、今度は l1, l2 が None だったときにどういう振る舞いをして欲しいんでしたっけ、と考えることになります。唯一絶対の答えはこういうのはないので、すべてはそういう考え方もあるのか、くらいでいいのです。)
| node = sentinel | ||
| carry = 0 | ||
|
|
||
| while l1 is not None or l2 is not None or carry != 0: |
6 participants
https://leetcode.com/problems/add-two-numbers/description/
Next: https://leetcode.com/problems/valid-parentheses/