-
Notifications
You must be signed in to change notification settings - Fork 0
21. Merge Two Sorted Lists #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
huyfififi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jupyter notebookでのPR生成は新鮮で面白いですね!
|
|
||
| ```python | ||
| def mergeTwoLists(self, list1: Optional[ListNode], list2: Optional[ListNode]) -> Optional[ListNode]: | ||
| if list1 is None or list2 is None: return list1 or list2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIが提示してきたものかと思いますが、この一行で (list1, list2) = (None, None), (None, not None), (not None, None), (not None, not None) 4通りがカバーされていて、それぞれ正しく動作するのか頭の中でシミュレーションをするのに少し時間がかかりました。
他の方々がどう思うかは分かりませんが、私はやや認知負荷が高く感じました。私は1行の持つ意味は少ない方が好みです。
| tail = tail.next | ||
| smaller = smaller.next | ||
|
|
||
| tail.next = bigger or smaller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私は、ごく個人的に、値をorで繋ぐことに少し怖さを感じるんですよね。今回の問題設定では大丈夫なのですが、もしListNodeが__bool__を定義していたら(または将来的に定義されたら)予期せぬ挙動をする可能性があるからです。コードレビューをする際にも、クラスの定義を一応確認しにいかなければならないので、ちょっとだけ手間が増えるような気がします(余計な心配かもしれませんが😅)。
In [1]: class ListNode:
...: def __init__(self, val=0, next=None):
...: self.val = val
...: self.next = next
...:
...: def __bool__(self):
...: return self.val > 0
...:
In [2]: bigger = ListNode(0)
In [3]: smaller = None
In [4]: (bigger or smaller) is None # biggerはListNodeなのにFalse判定されてsmallerのNoneに
Out[4]: True
In [5]: bigger = ListNode(1)
In [6]: (bigger or smaller) is None
Out[6]: False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。
orについて自分が勘違いしていて、= bigger or smallerはbiggerがNoneならsmallerを使うという意味かと思っていましたが、biggerがimplicit falseならsmallerを使う、という方が正確なんですね。
となると、biggerのspecial method __bool__の実装次第になったり、意図しないimplicit falseなどが生じうるので、おっしゃる不安はごもっともだと思います。
そのあたり十分理解せず使っていたので気をつけたいです。
| 3つのif文は1つにまとめられます。`list1`がNoneなら`list2`を返す(`list2`もNoneならNoneが返る)、そうでなければ`list1`を返す: | ||
|
|
||
| ```python | ||
| if list1 is None or list2 is None: return list1 or list2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私はこれあんまり好きでないですね。違うものをわざわざ混ぜて判定して混ぜて返しているので。
一方1つ目の条件は消せますね。
if list1 is None:
return list2
if list2 is None:
return list1また、C++ は a || b が true か false になるなど言語によってはこれが書けないので注意です。
|
|
||
| **末尾の残り連結** | ||
|
|
||
| `node1 or node2`で残っている方を取得できます: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perl などインタープリター言語ではこう書けますが言語によってはできないですね。
C++でも node1 ? node1 : node2 はあるわけですが、うーん、まあ、趣味の範囲ですかね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。
確かに、= node1 or node2は響きとしてboolっぽく、pythonなどのインタープリタ言語特有なので他言語がメインの人には認知負荷が高いかもしれません。そういったところを理解し、少なくとももっと注意して使うようにしたいです。
|
レビューを受けてStep 4を追加。 |
21. Merge Two Sorted Lists
Grind 75です。jupyter notebookで草稿を作りPRを自動生成する方式にしてみました。
Next: 125. Valid Palindrome