Conversation
| ListNode* seek = head; | ||
| ListNode* pre = NULL; | ||
| ListNode* border; | ||
| int sequence_cnt; |
There was a problem hiding this comment.
これらの変数がどんな役割を担うのか変数名のみからは分からなかったです。日本語でそれぞれの変数の役割を説明してもらえますか？
There was a problem hiding this comment.
seek:=前からみたとき、あるvalが最初に出現するノード。例えば[1,1,2,2]がInputなら、1回目のループはindnxが0のノード、2回目のループではindexが2のノードです。
pre=:= seekの一つ前のノード。seekが headのときはNULLです。
boundary: := 前からみたとき、あるvalが最後に出現するノード。例えば[1,1,2,2]がInputなら、1回目のループはindnxが1のノード、2回目のループではindexが3のノードです。
There was a problem hiding this comment.
seek:=前からみたとき、あるvalが最初に出現するノード。例えば[1,1,2,2]がInputなら、1回目のループはindnxが0のノード、2回目のループではindexが1のノードです。
index 0とindex 2ですか？
sequence_cntはどうでしょうか？
There was a problem hiding this comment.
読み手に変数の意味を推理させるコードになっているように思います。コメントをつけるか、意味が明らかな変数を使うようにしましょう。
There was a problem hiding this comment.
index 0とindex 2ですか？
修正しました。
sequence_cnt:=あるvalのリストの長さ。例えば[1,2,2]なら、1回目のループは1、2回目のループでは2です。
There was a problem hiding this comment.
ありがとうございます。
確かに言われてわかりにくいなと思いました。
次から以下のようにつけます。
seek → seek_start
border → seek_end
pre → pre_seek_end
There was a problem hiding this comment.
別論点の質問なのですが、今回のように全体のアルゴリズムが少し複雑な場合、変数名（変数の動き）を明確にしても依然全体のアルゴリズムがわかりにくく、困っています。
認知負荷を高めず全体の動きを伝えるために、何か良い伝達方法はありますか。
（次からは複雑な処理に少しコメントを書こうと考えています。）
There was a problem hiding this comment.
sequence_cnt:=あるvalのリストの長さ。例えば[1,2,2]なら、1回目のループは1、2回目のループでは2です。
L25でif (sequence_cnt == 0) {となっていますが、この説明であっていますか？
seek → seek_start
border → seek_end
pre → pre_seek_end
L26でpre = seek;とありますが、この変数名で大丈夫ですか？
今回の場合は処理が不要に複雑になっていると思います。自分で上手く説明できない変数を使っているのも一因かと感じました。簡潔な方法を考えてみると良いでしょう。
class Solution {
public:
ListNode* deleteDuplicates(ListNode* head) {
ListNode dummy;
dummy.next = head;
auto node = &dummy;
while (node->next && node->next->next) {
if (node->next->val != node->next->next->val) {
node = node->next;
continue;
}
int duplicate_val = node->next->val;
auto next = node->next->next->next;
while (next && next->val == duplicate_val) {
next = next->next;
}
node->next = next;
}
return dummy.next;
}
};There was a problem hiding this comment.
確かに、私の説明はその二つに答えられていません。
質問されて、かなりニュアンスで実装したことに気づきました。
その実装はとてもシンプルですね。
確かに「今見ている値が重複しているか」をガード条件にすれば、 sequence_cntは必要ないですし、whileの下部（ガード条件の後のメイン部分）にwhileを置けます。
アルゴリズムは同じでも、もっとシンプルな構造で書けないか考えます。
ありがとうございます。
| if (!pre) { | ||
| seek = head = border->next; | ||
| } | ||
| else { | ||
| pre->next = border->next; | ||
| seek = border->next; | ||
| } |
There was a problem hiding this comment.
一方では「=を2つ使って１行に書いてる」のに、もう一方では「2行に分けている」というところで一貫性がないように感じました。
もし2行に分けるのであれば、seek = border->next;は共通してるのでif文の外に出してもいいと思いました。
| class Solution { | ||
| public: | ||
| ListNode* deleteDuplicates(ListNode* head) { | ||
| std::map<int, int> cnts; |
There was a problem hiding this comment.
読む人の認知負荷を下げるため、単語から文字を抜いて略語にするのは避けることをお勧めいたします。
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
Do not abbreviate by deleting letters within a word.
また、 key と value がそれぞれ何を表すかを変数名で示すことをお勧めいたします。 value_to_frequency・value_to_frequencies はいかがでしょうか？
There was a problem hiding this comment.
cnt, tmp, mp, stあたりをよく使ってしまうため、気をつけます。
また、勉強になるリンクをありがとうございます。拝見させていただきます。
とても良いmapの命名ルールをありがとうございます。
確かにそれら命名なら、関数の始まりで定義しても何を格納するか明確に分かります。
また、今回のケースならcountをfrequencyと表現できる点も勉強になりました。
追記：意味的にもmap::countがあることからも、frequencyが良いのですね。
| cnts[head->val]++; | ||
| head = head->next; | ||
| } | ||
| ListNode *deleted_head = NULL; |
There was a problem hiding this comment.
deleted_head は head が削除されているというニュアンスを感じます。 head_of_list_without_duplications であれば、何を表しているか叙述的になると思います。ただ、長いようにも感じます。 head_without_duplications だと、ヘッドに重複がないと誤解される可能性がありますが、ギリギリ大丈夫かと思います。
There was a problem hiding this comment.
前置詞を使った命名を思いつかなかったのでとても新鮮でした。
書いていたときも、deleted_head のニュアンスがおかしいので、deleted_list_headなどにしようと考えていました。
head_without_duplicationsは単に「削除」だけでなく、「何を削除したのか」も明確になり、意味の密度が高いです。
良いオプションを提示していただきありがとうございます。
| head = head->next; | ||
| } | ||
| ListNode *deleted_head = NULL; | ||
| for (auto [n, c] : cnts) { |
There was a problem hiding this comment.
数行程度のスコープであれば 1 文字変数はよいと思うのですが、今回はスコープがやや長いため、叙述的な変数名を付けたほうが良いと思います。それぞれ value と frequency あたりが良いと思います。
count は std::count と重複するため、避けたほうが良いかもしれません。
There was a problem hiding this comment.
ありがとうございます。
1 文字変数が機能するのが数行スコープという点を初めて知りました。（よく見る処理は1文字で命名する悪癖がついていました。）
スコープが長いときは叙述的な変数名を付けます。
また、ライブラリの他の関数と命名の重複を避けるという視点も新鮮でした。
| border = seek; | ||
| sequence_cnt = 0; |
There was a problem hiding this comment.
この2つの変数は、while で引き継がないので、ループの中で宣言しましょう。
このコードが読みにくいのは、変数の役割が途中で変わるからです。
非常に大局的に説明すると、「seek の指す先と同じものがいくつ後ろに続いているかを数える」「1つ以上ならば消す」「0ならばその要素を取っておく」ですね。
pre は取っておくものが見つかるまでは null で見つかったら、見つかったら見つかった最後のものを指します。
head は取っておくものが見つかるまでは seek と一緒に動き続け、見つかったら見つかったはじめのものを指します。
pre は、null でまだ見つかっていないものを示す慣例があるからまだいいとしても、この head は日本語で書いても50文字です。これを2つに分けるとまだ読みやすくなります。あとは、関数にすることですね。
それから、他の人のコードをもう少し読む練習をして欲しいです。
https://note.com/nir29/n/n025ddc315a2e
There was a problem hiding this comment.
関数にするのはどう考えているかを整理することなので、実際のコードにするかはともかく考えるうえでもしたほうがいいでしょう。
int count_same(ListNode*);
... delete_n(int n, ListNode*); // 返り値は適当に考えてください。消したあとの次でも返しますか
... keep_node(ListNode* node_to_keep, ...);これをしておくと、わりと自然に、完成品の鎖の頭と尻尾が欲しくなるように思います。そしたら変数を用意したらよいでしょう。番兵というテクニックもあります。
それから、不変条件をなんだと思っているのか、つまり、仕事の引き継ぎだと思うと、どういう状態で次のループに引き継いでいるのかを明確にしたほうがいいかもしれません。(繋いでから切断、切断してから繋ぐ、とか。)
t0hsumi/leetcode#4 (comment)
取れる選択肢を増やしたほうがよさそうです。
There was a problem hiding this comment.
自然な流れで理解できました。
AtCoderで「whileの中で毎回使う変数は、whileの外で宣言すると宣言時のオーバーヘッドを減らせる」と学んだため、whileの外で宣言していました。
しかし、確かにこの書き方では、読み手は「この変数は次のループに引き継ぐんだな」と勘違いしてしまいます。
次からどういう状態で次のループに引き継いでいるのかを考えます。
また、関数に切り分けたら実際のコードに残すと思い込んでいたので、考えるための関数化は初めて知りました。
他の人のコードを見て選択肢を増やします。（この問題まで人のコードをほとんど読んでいませんでした。）
丁寧に教えていただきありがとうございます。
There was a problem hiding this comment.
AtCoderで「whileの中で毎回使う変数は、whileの外で宣言すると宣言時のオーバーヘッドを減らせる」と学んだ
これ、たしかに、コンストラクターが相当に重ければそうでしょうが、これが実際に問題になる状況はちょっと考えにくいです。(宣言時のオーバーヘッドの時間を見積もってみましょう。)
AtCoder わりと変な癖がつくので、レートともにだんだん能力が下がるんですよ。
https://docs.google.com/presentation/d/1Ny4kmHE2FZMI0AuPxImokweGoAE73RAGivjDJg0kG80/edit#slide=id.g2f39dd1a75f_141_0
There was a problem hiding this comment.
私の中で、この理由は最近言語化できた気がしています。
状況や問題に対して10個くらい気がつかなくてはいけない点、論点、つまり「ソフトウェアエンジニアリングの常識」があって、そのうちの8,9割が見えていれば、専門家として扱われるんですが、常識のうち、優先順位が低いところしか気が付かない上に、変なプラクティスを覚えるので論点に逆行します。
いや、こういった変なプラクティスは、論点が分かっていない人でも、たまに見かけるはまりどころにはまらないためにあるんで、意味がないわけじゃないんですが、どちらかというと問題は思考をしないように作られているところで、それが積極的に論点を見えなくします。
このあたり、我々、競技プログラミング同好会とその前後の世代が開発したものが結構あって、申し訳ないところなのですが。
There was a problem hiding this comment.
ありがとうございます。
確かに、関数呼び出し時のスタックフレームの挙動を考えたらそうです。
また、 timeで計測しても有意な差は見られませんでした。
（10^8ほどのループで「while内で毎回宣言」と「while外で一度だけ宣言」はマイクロ秒単位ではどちらも同じ秒数でした。）
ただ、癖を直すのは、自分が良いと感じ、過去に褒められていた事自体を疑うことなので、かなり強烈にメタ認知をする必要があり大変です。
まず、上の話を理解することが第一歩だと思うので、時々説明しています。
その上で、一つ一つの判断が仕事の目的に繋がっているかどうかを正当化できるか考え続けることになるでしょう。
痛感しています。
言語化されて気づきましたが、時々間違った報酬系を刺激し、仕事の目的とは別の方向に思考・行動していました。
今の思考・行動が仕事の目的に繋がっているかを批判的に考えるようにします。
There was a problem hiding this comment.
そうですね。実際に計測してみるのも大事ですが、統計誤差があるので中間出力を比べるのも一つです。
その2つ -S をつけてコンパイルしてみると最適化なしでも差がないようでした。
まあ、しかも、最近は、「ただの遊びであることが分かっている」というようになってきましたね。競技プログラミングの先輩たちがそういうからそういうようになってきたんですが、しかし、報酬系の調整までできない水準のメタ認知はむしろ修正が効かなくなるので、ないほうがまし(なのでつけるならば覚悟を持ってつけきらないといけない)という感覚ですね。
There was a problem hiding this comment.
There was a problem hiding this comment.
ありがとうございます。
確かに、最適化で差を生む可能性を考慮してませんでした。
中間出力で比較すれば差の意味を理解できます。
報酬系の調整までできない水準のメタ認知はむしろ修正が効かなくなるので、ないほうがまし(なのでつけるならば覚悟を持ってつけきらないといけない)という感覚ですね。
報酬系を自力でメタ認知し修正するのは難しいのでしょうね。
特にAtcoderからプログラミングにのめり込むと、他の界隈と関わりがないため「遊びである」などの情報が入らず（入ってきても感覚的に分からず）、報酬系を調整するための相対化ができないです。
（恥ずかしながら、私もレートを上げるために何が足りないかしかメタ認知できていませんでした。）
There was a problem hiding this comment.
「変数の役割が途中で変わる」ものは前にも言及ありました。
リンクをありがとうございます。
特に「変数の役割を自然言語で説明して違和感があるか」、「変数の値が変わること以上に意味が変わることが問題である」は直感的に重要だと理解しました。
確かにそれらに留意しないと、認知負荷を軽減するための命名が毒に変わりそうです。
それらの点に注意します。
5 participants
問題へのリンク
Remove Duplicates from Sorted List II
次に解く問題
Add Two Numbers