206. reverse linked list#7
Conversation
| ListNode* reversed_head = nullptr; | ||
| ListNode* cur_node = head; | ||
| while (cur_node) { | ||
| auto tmp = cur_node->next; |
There was a problem hiding this comment.
tmpという名前だけだと、cur_nodeにtmpを代入するときにあれtmpってなんだったっけ、とこの行に戻って確認する必要が出てきてしまう気がするので、next_nodeとかsuccessorなどより説明的な変数名を割り当てても良いと思いました。
There was a problem hiding this comment.
ありがとうございます。nextだとnextがそのループを抜けるとcurrentになり…とか考えると下手に意味を持たせないほうがいいかなと思いましたが、もっと距離が離れたりするとtmpではわからなくなりそうですね。successorは空間的なニュアンスがありわかりやすいですね。
There was a problem hiding this comment.
個人的には original_next_node などを使おうか考えたりしました。もっとよい命名はありそうだと思います。
| tail = tail->next; | ||
| } | ||
| // これってなんで必要？ | ||
| tail->next = nullptr; |
There was a problem hiding this comment.
これがないとreverseした最後のNodeと最後から二番目のNodeがお互いを指してcycleになる気がしますね
| st.push(tail); | ||
| tail = tail->next; | ||
| } | ||
| head = st.top(); |
There was a problem hiding this comment.
ざっと読んだ時に、この行を読み飛ばしていて、headは入力リストの先頭ではないのか？と一瞬混乱しました。
この関数内では、headは入力リストの先頭として意味を崩さず、20行目は、後の方の解法のようにreversed_headとした方がわかりやすく感じます。
There was a problem hiding this comment.
ありがとうございます。おっしゃる通り、最初書いたときは引数のheadを戻り値に流用していたのですが、一つのプログラム内で変数の意味合いが変わるのは紛らわしいと思いやめました。
| return node; | ||
| } | ||
| ListNode* head = reverseRecursively(node->next); | ||
| node->next->next = node; |
There was a problem hiding this comment.
これは個人の感想で他の方々の意見を伺いたいですが、nextのnextにアクセスするのって頭の中で画を想像しづらくて結構トリッキーだと感じるんですよね。返り値で(reversed_head, reversed_tail)の二つを返すか、引数で(reversing, appending)を受け取る方法 (参考)の方がなんとなく想像しやすい解法のような気がします。
There was a problem hiding this comment.
たしかにうまく再帰する部分問題に分けれていない感じがありますね…ご参考で頂いた書き方がスタイリッシュでいいですね！
// [1,2,3], [4,5,6] -> [3,2,1,4,5,6]
// [], [4,5,6] -> [4,5,6]
ListNode* reverse_and_append(ListNode* head1, ListNode* head2){
if (head1 == nullptr) {
return head2;
}
auto new_head1 = head1->next;
head1->next = head2;
return reverse_and_append(new_head1, head1);
}
| ListNode* reverseList(ListNode* head) { | ||
| std::stack<ListNode*> st; | ||
| auto sentinel = nullptr; | ||
| st.push(sentinel); |
| ListNode* reverseList(ListNode* head){ | ||
| // 反転済みリストの先頭 | ||
| ListNode* reversed_head = nullptr; | ||
| ListNode* cur_node = head; |
There was a problem hiding this comment.
cur_node という変数名は読む助けにほとんどならないのでもう少し良いものが欲しいですね。
「変数名をつける」という行為も、比較的小さいものであるにしても技術的選択であり、その技術的選択はなんらかの意味でエンジニアリングに繋げて判断する、ということです。
There was a problem hiding this comment.
ありがとうございます。いまいちcur_nodeがよくないという感覚が持てなかったのですが、現にこのコードで直後に変数名を間違えており意味のある命名の必要性を感じました。
| if (head == nullptr) { | ||
| return nullptr; | ||
| } | ||
| while(tail != nullptr) { |
There was a problem hiding this comment.
while のあとにスペースを空けることをお勧めします。
参考までにスタイルガイドへのリンクを貼ります。
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace
if (b) { // Space after the keyword in conditions and loops.
ただし、上記のスタイルガイドは唯一絶対のルールではなく、複数あるスタイルガイドの一つに過ぎないということを念頭に置くことをお勧めします。また、所属するチームにより何が良いとされているかは変わります。自分の中で良い書き方の基準を持ちつつ、チームの平均的な書き方で書くことをお勧めいたします。
There was a problem hiding this comment.
たびたびご指摘頂きすみません。自分の中でルールを決めて気づけるように気をつけます。
| if (node == nullptr || node->next == nullptr) { | ||
| return node; | ||
| } | ||
| ListNode* head = reverseRecursively(node->next); |
6 participants
This Problem
https://leetcode.com/problems/reverse-linked-list/