Conversation
|
|
||
| 変数名がなかなか悩みどころ。 | ||
|
|
||
| `prev`, `curr`, `next`だと、reverseしたlistにおける"前"や"次"なのか、元のlistにおける"前"や"次"なのか少し混乱する。 |
There was a problem hiding this comment.
STEP3では改善されていたと思いますが、意味と形式のどちらから名前を付けるか、混乱する場合は両方試して比較してみると良いと思いました。
t0hsumi/leetcode#7 (comment)
| # self.next = next | ||
| class Solution: | ||
| def reverseList(self, head: Optional[ListNode]) -> Optional[ListNode]: | ||
| def reverse_list_helper( |
There was a problem hiding this comment.
L12 のコメントのせいで見にくくなっていると思いました。以下のように別の行に書くのもありでしょうか。
def reverse_list_helper(
node: Optional[ListNode],
) -> tuple[Optional[ListNode], Optional[ListNode]]:
# Returns: reversed_tail, reversed_headThere was a problem hiding this comment.
確かに、どうせ返り値の説明を書くなら素直にdocstringを書いた方がいい気がしてきました。ありがとうございます!
|
|
||
| `prev`, `curr`, `next`だと、reverseしたlistにおける"前"や"次"なのか、元のlistにおける"前"や"次"なのか少し混乱する。 | ||
|
|
||
| また、処理をどうグループ化するかも悩ましい。while文の中に4つの短い処理をスペース無しで並べると、それらに順序的な関係がないような印象を覚えてしまうので (偏った感覚?)、スペースを入れたいのだが |
There was a problem hiding this comment.
個人的には、全体で数行の規模なので空行はそこまで読みやすさに寄与しないかなと思いました。
| class Solution: | ||
| def reverseList(self, head: Optional[ListNode]) -> Optional[ListNode]: | ||
| def reverse_list_helper( | ||
| node: Optional[ListNode], |
There was a problem hiding this comment.
個人的には、このぐらい複雑な処理になるとnode以上の名前をつけてもいい気がしました。(reversing_nodeなど)
There was a problem hiding this comment.
reversing_nodeという変数名いいですね!参考になります
|
|
||
| node = next_node_to_reverse | ||
|
|
||
| return reversed_head |
There was a problem hiding this comment.
個人的な感覚ですが、最近はせっかく空行を入れるならそこにコメントを積極的に入れてもいいような気がしてきました。何かしらの区切りであることはわかるのですが、空行だけだとどういう区切りかが上から読んでいったときにわからないので。(関数名や構文などから明らかにどういう区切りかわかる場合は別ですが!)
There was a problem hiding this comment.
確かに、解いている間はいいのですが、記憶を消してコードを眺めてみるとどういった区切りなのかわかりにくいですね。ハッとさせられる指摘でした、ありがとうございます!
|
|
||
| while node: | ||
| next_node_to_reverse = node.next | ||
|
|
There was a problem hiding this comment.
個人的にはもう少し空行がない方が好みです。参考↓
https://pep8-ja.readthedocs.io/ja/latest/#id11
関数の中では、ロジックの境目を示すために、空行を控えめに使うようにします。
https://google.github.io/styleguide/pyguide.html#35-blank-lines
There was a problem hiding this comment.
気になって原文だとどういう表現か見てみたら、確かに
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 |
There was a problem hiding this comment.
今注目しているのも、という意味でのnodeという変数名の他にも、これから作業しなければならない列の先頭と言った意味で、restやrest_headなどというのも候補になりそうです。
There was a problem hiding this comment.
ありがとうございます!確かにrest*ならnodeよりも具体的に何を指しているのか情報が載せられていますね。あまり使用例を見たことがないのですが、後で調べてみます!
| if node.next is None: | ||
| return node, node | ||
|
|
||
| reversed_tail, reversed_head = reverse_list_helper(node.next) |
There was a problem hiding this comment.
(私の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( |
There was a problem hiding this comment.
この関数が何をするつもりなのか、少し日本語で表現してもらえませんか。
「XXXX と YYYY を受け取って、ZZZZ にしたものを返す」だとすると、「YYYY をひっくり返して、後ろに XXXX をつけたもの」というのであっていますか? 読み取るのが結構大変に思います。
さらに curr_node がひっくり返すと後ろになるということを利用しているなどの事情もあるでしょう。
There was a problem hiding this comment.
ありがとうございます。
はい、「XとYを受け取って、Y以降を反転させ、その末尾にXを繋げたリストの先頭を返す」という理解です。
ひっくり返す操作によって、元のリストのcurrent基準から見たpreviousは、逆順にしたリストの視点からcurrent基準で見るとnextになってしまい、(curr_nodeも同様に)ある行においてどちらの視点で見るべきなのか、自分でも混乱しがちでした。prev_nodeがこの時点ではひっくり返らないのも、一見腑に落ちにくい気がします。
There was a problem hiding this comment.
いや、素直に呼ぶと、「続きの部分」と「ひっくり返す部分」を受け取って、「ひっくり返す部分をひっくり返して、続きの部分をくっつける」ではないですか。これを「前」と「今」とか読んでいるんですよ。で、さらに「ヘルパーする」と名前をつけていますね。
「ヘルパー」するとは、次のことである。
「前」と「今」を受取り、「今」がなければ「前」を返す。「今の先頭」と「今の続き」で「ヘルパー」したものを「ひっくり返った頭」とする。今の次を「前」にする。「ひっくり返った頭」を返す。
これは正気ではないでしょう。
「続きの部分」と「ひっくり返す部分」を受取り、「ひっくり返す部分」がなければ「続きの部分を」返す。
「「ひっくり返す部分の続き」をひっくり返して、後ろに「ひっくり返す部分の先頭」をくっつける」をして、「ひっくり返った頭」を手に入れる。「ひっくり返す部分」の後ろに「続きの部分」をつける。「ひっくり返った頭」を返す。
まだ分かります。
There was a problem hiding this comment.
少し混乱していました。元のリストの末尾側からひっくり返していくので、この関数で(X, Y)を受け取ると、Xが未処理・続きの部分、Yがひっくり返す部分になり、そのXとYに「前」、「今」と名付けるのはあまりに不適当ですね。
一つ目の引数をrestやnext_to_reverse、二つ目の引数をreversing_nodeと名付けるべきだったと思いました。
ヘルパー関数の命名については、今すぐにはうまい代替案が思いつきませんが、少し考えてみます。ありがとうございます!
There was a problem hiding this comment.
問題として
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読めますか。また、これさらに変形したくなりませんか。
There was a problem hiding this comment.
「前」「今」という関数の引数の命名方法、関数の機能とは関係がなくて、自分が典型的にどうやって出会うかですね。そういう名前を引数につけても、あまり読む助けになりません。
料理も手続きなので、どのようにそういった場面でどう説明するかを考えるといいかもしれません。
ナス、トマトのような一般名詞、あるいは、サラダ用の野菜、カレー用の野菜などの用途あたりではないですか。入手場所や方法で呼びませんね。デパートとか通販とか。
There was a problem hiding this comment.
料理のアナロジー、ありがとうございます。前の場面のコンテクストを引きずった命名をしてしまったことを反省しています。
いただいたコードですが、前の部分がひっくり返す操作、後ろの部分がくっつける操作になっていて、一つの関数内に二つの責務があることが強調されていると感じました。反転・連結の操作によって頭を揺らされる感覚があったので、関数を分けて操作を分離・またはシグネチャを変えて関数内の操作を単純化したいなと思いました。
違和を感じる能力にまだ課題があり、質問の意図を誤解している気がします。お手数ですが、よろしければもう少しヒントをいただければ嬉しいです。
There was a problem hiding this comment.
上のコードはこうしませんか。代入を前に持っていきます。そうするとさらに末尾再帰の形なのでループにできますね。
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)There was a problem hiding this comment.
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 []この問題の裏テーマとして末尾再帰があるように思いました。再帰 -> 末尾再帰 -> ループを意識していこうと思います。辛抱強く付き合っていただいてありがとうございます!
There was a problem hiding this comment.
はい。そのコード step1_iterative.py とほぼ同じですね。こういう風に色々な変形が念頭にあって書いてます。
b6187c8 to
51276ff
Compare
f45454a to
e6d6b0e
Compare
https://leetcode.com/problems/reverse-linked-list/description/