-
Notifications
You must be signed in to change notification settings - Fork 0
Solve 2_add_two_numbers_medium #10
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
maeken4
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.
全体的に読みやすいと思います。ただ、人によってはPythonの三項演算子は好まれないため避けたほうが無難かもしれません。
digit1 = 0
if l1:
digit1 += l1.val
l1 = l1.next
等の書き方でも十分簡潔かと思います。
|
レビューありがとうございます。 |
| ) -> Optional[ListNode]: | ||
|
|
||
| def add_two_list_nodes( | ||
| l1: Optional[ListNode], l2: Optional[ListNode], carry_of_digits: int |
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.
単に carry でも十分に通じると思いました。
| def add_two_list_nodes( | ||
| l1: Optional[ListNode], l2: Optional[ListNode], carry_of_digits: int | ||
| ) -> Optional[ListNode]: | ||
| if not l1 and not l2 and carry_of_digits == 0: |
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.
if l1 is None and l2 is None and carry_of_digits == 0: と書いてもよいと思いました。
参考までにスタイルガイドへのリンクを貼ります。
https://google.github.io/styleguide/pyguide.html#2144-decision
Always use if foo is None: (or is not None) to check for a None value. E.g., when testing whether a variable or argument that defaults to None was set to some other value. The other value might be a value that’s false in a boolean context!
ただし、上記のスタイルガイドは唯一絶対のルールではなく、複数あるスタイルガイドの一つに過ぎないということを念頭に置くことをお勧めします。また、所属するチームにより何が良いとされているかは変わります。自分の中で良い書き方の基準を持ちつつ、チームの平均的な書き方で書くことをお勧めいたします。
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.
レビューありがとうございます。
なるほど、こういうオブジェクトのNoneの判定には基本的にはisを使うようにします。
リストのようなイテレータオブジェクトで、空の場合とNoneの場合も区別せずに処理したい場合には if not x:を使うようにします。
| ) -> Optional[ListNode]: | ||
| if not l1 and not l2 and carry_of_digits == 0: | ||
| return None | ||
| elif not l1 and not l2: |
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.
step 2 のように
if not l1:
l1 = ListNode()
if not l2:
l2 = ListNode()と書いたほうがシンプルだと思いました。
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.
ありがとうございます。step2で改善できたようで良かったです。
| ) -> Optional[ListNode]: | ||
| if not l1 and not l2 and carry_of_digits == 0: | ||
| return None | ||
| elif not l1 and not l2: |
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.
直前が return None と関数の終わりを表しているため、 step 2 のように if のほうがよいと思いました。
| digit1 = l1.val if l1 else 0 | ||
| digit2 = l2.val if l2 else 0 | ||
| total = digit1 + digit2 + carry_of_digits | ||
| carry_of_digits, digit = divmod(total, BASE_NUMBER) |
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.
個人的には
carry = total // BASE_NUMBER
digit = total % BASE_NUMBERの方が unpack より読み下しやすいと感じてしまうのですが、単に私が divmod をあまり使わないので見慣れていないだけかもしれません。
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.
レビューありがとうございます。
結論、可読性とパフォーマンスとのトレードオフかと思います。
divmodの方が//と%よりも演算単体のパフォーマンスが高い(後述)のですが、それが全体のパフォーマンスにどれほど影響するかといわれると状況によります。(というかむしろ、ほかの部分がボトルネックになることがほとんどかと思います。実際に計算したところ、相当巨大な数を何度も計算する場合などに限られると感じました。)
一方で、それ以上に可読性が下がってしまうのがまずいとすると、後者を優先するべきだと思います。
divmodが一般的かどうか、読みづらくなるかどうかもチームにはよるかと思うのですが、このあたりは他の方の所感も気になります。
【補足】
バイトコードレベルでみると、割り算の結果とその余りは同時に得られるので、divmodの方が//と%を二度使うよりも単純に二倍パフォーマンスが高いです。
実際、以下のような結果が得られます。
>>> import timeit
>>> timeit.timeit('divmod(n, d)', 'n, d = 2**74207281 - 1, ', number=100)
0.8267046669998308
>>> timeit.timeit('n // d, n % d', 'n, d = 2**74207281 - 1, 26', number=100)
1.654150374999972
参考
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.
私も divmod() に見慣れていないだけかもしれませんが、個人的には今回の問題の条件では性能よりも可読性を選択すると思いました。
| # visit each node iteratively and create added digit one by one | ||
| # break the loop if two nodes l1 and l2 are None and no carry of digits |
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.
(自分用のメモ、というだけであれば恐縮ですが) このコメントはコードを読めば明らかにわかることで、特段複雑な処理をしているわけでもありませんし、冗長なように感じます。コメントは何をしているかよりも、なぜそうしているのか、を言及することが多いかと思います。
たとえば "10" というマジックナンバーが出てきたとき、BASE_NUMBER = 10 のように変数として宣言して命名するか、もしくは一度しか使わないようなものなら当該箇所に # Using base number 10 などつけると思います。
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.
確かにそのコメントは不要に感じました。ありがとうございます。
問題へのリンク
Add Two Numbers - LeetCode
言語
Python
問題の概要
与えられた2つの非負整数を逆順で表現した連結リストとして受け取り、それらの合計を同様に連結リストで返す問題。
自分の解法
再帰関数を用いた解法。
連結リストl1, l2の長さをそれぞれ
m,nとするとO(max(m,n))O(max(m,n))(再帰のスタック領域を含む)step2
再帰的な実装から反復的な実装に変えた。また、変数名もより明確なものに変更した。
dummy_headノードが導入された。これにより、リストの先頭ノードに関する特別な処理が不要になり、コードが単純化されている。Noneノードの処理方法の改善Noneの場合に新しいListNodeを生成していた。step2では、l1.val if l1 else 0という三項演算子を用いることで、この処理をより効率的かつ簡潔に記述している。while l1 or l2 or carry_of_digits:という単一の明快なループ条件に集約された。これは、処理を継続すべき全てのケースを網羅している。別解・模範解答
再帰関数ではなく、ループを用いてノードを1つずつ処理する解法。
最後に返すノードは連結リストの先頭ノードなので、ダミーノードを用意しておき、そこから結果の連結リストを構築する。その後、ダミーノードの次のノード(
dummy_head.next)を返す。ループの最中には、今自分がどのノードを処理しているかをきちんと把握しておく。コードの中でもそれが明示できるように心がける。今回ならば、while文の中で足し合わせたノードは
node.nextに保存する。連結リストl1, l2の長さをそれぞれ
m,nとするとO(max(m,n))O(1)次に解く問題の予告