Skip to content

206. Reverse Linked List #18

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

Merged
merged 20 commits into from
May 23, 2025
Merged

206. Reverse Linked List #18

merged 20 commits into from
May 23, 2025

Conversation

huyfififi
Copy link
Owner

@huyfififi huyfififi self-assigned this May 17, 2025
@huyfififi huyfififi marked this pull request as ready for review May 17, 2025 04:24

変数名がなかなか悩みどころ。

`prev`, `curr`, `next`だと、reverseしたlistにおける"前"や"次"なのか、元のlistにおける"前"や"次"なのか少し混乱する。

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STEP3では改善されていたと思いますが、意味と形式のどちらから名前を付けるか、混乱する場合は両方試して比較してみると良いと思いました。
t0hsumi/leetcode#7 (comment)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

参考リンクありがとうございます!

# self.next = next
class Solution:
def reverseList(self, head: Optional[ListNode]) -> Optional[ListNode]:
def reverse_list_helper(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L12 のコメントのせいで見にくくなっていると思いました。以下のように別の行に書くのもありでしょうか。

        def reverse_list_helper(
            node: Optional[ListNode],
        ) -> tuple[Optional[ListNode], Optional[ListNode]]:
            # Returns: reversed_tail, reversed_head

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かに、どうせ返り値の説明を書くなら素直にdocstringを書いた方がいい気がしてきました。ありがとうございます!


`prev`, `curr`, `next`だと、reverseしたlistにおける"前"や"次"なのか、元のlistにおける"前"や"次"なのか少し混乱する。

また、処理をどうグループ化するかも悩ましい。while文の中に4つの短い処理をスペース無しで並べると、それらに順序的な関係がないような印象を覚えてしまうので (偏った感覚?)、スペースを入れたいのだが

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的には、全体で数行の規模なので空行はそこまで読みやすさに寄与しないかなと思いました。

class Solution:
def reverseList(self, head: Optional[ListNode]) -> Optional[ListNode]:
def reverse_list_helper(
node: Optional[ListNode],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的には、このぐらい複雑な処理になるとnode以上の名前をつけてもいい気がしました。(reversing_nodeなど)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reversing_nodeという変数名いいですね!参考になります


node = next_node_to_reverse

return reversed_head
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的な感覚ですが、最近はせっかく空行を入れるならそこにコメントを積極的に入れてもいいような気がしてきました。何かしらの区切りであることはわかるのですが、空行だけだとどういう区切りかが上から読んでいったときにわからないので。(関数名や構文などから明らかにどういう区切りかわかる場合は別ですが!)

Copy link
Owner Author

@huyfififi huyfififi May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かに、解いている間はいいのですが、記憶を消してコードを眺めてみるとどういった区切りなのかわかりにくいですね。ハッとさせられる指摘でした、ありがとうございます!


while node:
next_node_to_reverse = node.next

Copy link

@h1rosaka h1rosaka May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的にはもう少し空行がない方が好みです。参考↓

https://pep8-ja.readthedocs.io/ja/latest/#id11

関数の中では、ロジックの境目を示すために、空行を控えめに使うようにします。

https://google.github.io/styleguide/pyguide.html#35-blank-lines

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

気になって原文だとどういう表現か見てみたら、確かに

Extra blank lines may be used (sparingly) to separate groups of related functions.

"(sparingly)"という文が入っていました。PEP8の著者たちがどれくらいのニュアンスで入れたのか測りかねますが、意味をあまり持たない空行は無くすべきとのご指摘、その通りだと思います。

Googleのスタイルガイドもありがとうございます!

Use single blank lines as you judge appropriate within functions or methods.

このあたりの感度を高めていけるといいなと思いました。

class Solution:
def reverseList(self, head: Optional[ListNode]) -> Optional[ListNode]:
reversed_head = None
node = head

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

今注目しているのも、という意味でのnodeという変数名の他にも、これから作業しなければならない列の先頭と言った意味で、restrest_headなどというのも候補になりそうです。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます!確かにrest*ならnodeよりも具体的に何を指しているのか情報が載せられていますね。あまり使用例を見たことがないのですが、後で調べてみます!

if node.next is None:
return node, node

reversed_tail, reversed_head = reverse_list_helper(node.next)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(私のPR見てくださったようなのでご存知かもですが、)実はtailを受け取らずに解く方法もあります。
https://github.com/h1rosaka/arai60/pull/10/files#diff-f1530fc1072ee1f0b7de99a2e5236992c72355da69982c8ca516fcfba7c57927R122

# self.next = next
class Solution:
def reverseList(self, head: Optional[ListNode]) -> Optional[ListNode]:
def reverse_list_helper(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この関数が何をするつもりなのか、少し日本語で表現してもらえませんか。
「XXXX と YYYY を受け取って、ZZZZ にしたものを返す」だとすると、「YYYY をひっくり返して、後ろに XXXX をつけたもの」というのであっていますか? 読み取るのが結構大変に思います。

さらに curr_node がひっくり返すと後ろになるということを利用しているなどの事情もあるでしょう。

Copy link
Owner Author

@huyfififi huyfififi May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます。
はい、「XとYを受け取って、Y以降を反転させ、その末尾にXを繋げたリストの先頭を返す」という理解です。

ひっくり返す操作によって、元のリストのcurrent基準から見たpreviousは、逆順にしたリストの視点からcurrent基準で見るとnextになってしまい、(curr_nodeも同様に)ある行においてどちらの視点で見るべきなのか、自分でも混乱しがちでした。prev_nodeがこの時点ではひっくり返らないのも、一見腑に落ちにくい気がします。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いや、素直に呼ぶと、「続きの部分」と「ひっくり返す部分」を受け取って、「ひっくり返す部分をひっくり返して、続きの部分をくっつける」ではないですか。これを「前」と「今」とか読んでいるんですよ。で、さらに「ヘルパーする」と名前をつけていますね。

「ヘルパー」するとは、次のことである。
「前」と「今」を受取り、「今」がなければ「前」を返す。「今の先頭」と「今の続き」で「ヘルパー」したものを「ひっくり返った頭」とする。今の次を「前」にする。「ひっくり返った頭」を返す。

これは正気ではないでしょう。

「続きの部分」と「ひっくり返す部分」を受取り、「ひっくり返す部分」がなければ「続きの部分を」返す。
「「ひっくり返す部分の続き」をひっくり返して、後ろに「ひっくり返す部分の先頭」をくっつける」をして、「ひっくり返った頭」を手に入れる。「ひっくり返す部分」の後ろに「続きの部分」をつける。「ひっくり返った頭」を返す。

まだ分かります。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

少し混乱していました。元のリストの末尾側からひっくり返していくので、この関数で(X, Y)を受け取ると、Xが未処理・続きの部分、Yがひっくり返す部分になり、そのXとYに「前」、「今」と名付けるのはあまりに不適当ですね。
一つ目の引数をrestnext_to_reverse、二つ目の引数をreversing_nodeと名付けるべきだったと思いました。
ヘルパー関数の命名については、今すぐにはうまい代替案が思いつきませんが、少し考えてみます。ありがとうございます!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

問題として
reversed_head = reverse_list_helper(curr_node, curr_node.next)
が分からないのです。後ろに足すのが第一引数なのもややこしいです。

いいかはともかく逆方向に極端なことをすると

def reverse_the_first_and_append_the_second(reversing, appending):
    if reversing is None:
        return appending
    reversing_tails = reversing.next
    reversing.next = None
    combined = reverse_the_first_and_append_the_second(reversing_tails, reversing)
    reversing.next = appending
    return combined

読めますか。また、これさらに変形したくなりませんか。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「前」「今」という関数の引数の命名方法、関数の機能とは関係がなくて、自分が典型的にどうやって出会うかですね。そういう名前を引数につけても、あまり読む助けになりません。

料理も手続きなので、どのようにそういった場面でどう説明するかを考えるといいかもしれません。
ナス、トマトのような一般名詞、あるいは、サラダ用の野菜、カレー用の野菜などの用途あたりではないですか。入手場所や方法で呼びませんね。デパートとか通販とか。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

料理のアナロジー、ありがとうございます。前の場面のコンテクストを引きずった命名をしてしまったことを反省しています。

いただいたコードですが、前の部分がひっくり返す操作、後ろの部分がくっつける操作になっていて、一つの関数内に二つの責務があることが強調されていると感じました。反転・連結の操作によって頭を揺らされる感覚があったので、関数を分けて操作を分離・またはシグネチャを変えて関数内の操作を単純化したいなと思いました。
違和を感じる能力にまだ課題があり、質問の意図を誤解している気がします。お手数ですが、よろしければもう少しヒントをいただければ嬉しいです。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上のコードはこうしませんか。代入を前に持っていきます。そうするとさらに末尾再帰の形なのでループにできますね。

def reverse_the_first_and_append_the_second(reversing, appending):
    if reversing is None:
        return appending
    reversing_tails = reversing.next
    reversing.next = appending
    return reverse_the_first_and_append_the_second(reversing_tails, reversing)

Copy link
Owner Author

@huyfififi huyfififi May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1: 末尾再帰の形にできる、2: 末尾再帰はループにできる という思考ステップの回路が繋がっていませんでした。確かに、いただいたコードをループにすることができました。

def reverse_the_first_and_append_the_second(reversing, appending):
    while reversing is not None:
        reversing_tails = reversing.next
        reversing.next = appending
        reversing, appending = reversing_tails, reversing
    return appending

末尾再帰と末尾呼出し最適化について調べる中で、OCamlのList Reversalの実装を確認したところ、いただいた末尾再帰の形のコードとほぼ一致していることに気がつきました。

let rec rev_append l1 l2 =
  match l1 with
    [] -> l2
  | a :: l -> rev_append l (a :: l2)

let rev l = rev_append l []

この問題の裏テーマとして末尾再帰があるように思いました。再帰 -> 末尾再帰 -> ループを意識していこうと思います。辛抱強く付き合っていただいてありがとうございます!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

はい。そのコード step1_iterative.py とほぼ同じですね。こういう風に色々な変形が念頭にあって書いてます。

@huyfififi huyfififi force-pushed the leetcode-206-reverse-linked-list branch from b6187c8 to 51276ff Compare May 21, 2025 10:58
@huyfififi huyfififi force-pushed the leetcode-206-reverse-linked-list branch from f45454a to e6d6b0e Compare May 21, 2025 23:44
@huyfififi huyfififi merged commit 524caca into main May 23, 2025
@huyfififi huyfififi deleted the leetcode-206-reverse-linked-list branch May 23, 2025 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants