-
Notifications
You must be signed in to change notification settings - Fork 0
Remove Duplicates from Sorted List II #4
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
## step1 | ||
- 出現が1回の要素から成る新しいリストを返す。 | ||
- 時間計算量O(Nlog(N))、空間計算量O(N)。 | ||
- mapのイテレーションは型が明確なのでautoを使う。 | ||
|
||
## step2 | ||
- 線形に探索し、被りがあればheadを変更すると、時間計算量O(N)、空間計算量O(1)になることに気づき、実装する。 | ||
- 用途などはあまり分からず、パフォーマンス要件の考察は断念 | ||
- 「複数回呼び出される関数に見えるので、どちらの計算量は少ない方が良い」という当たり前の考察しかできなかった。 | ||
|
||
## step3 | ||
- 3分ほどで実装。Step2でループごとの変数の動きをしっかり理解したため詰まらず実装できた。 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
#include <map> | ||
|
||
struct ListNode { | ||
int val; | ||
ListNode *next; | ||
ListNode() : val(0), next(nullptr) {} | ||
ListNode(int x) : val(x), next(nullptr) {} | ||
ListNode(int x, ListNode *next) : val(x), next(next) {} | ||
}; | ||
|
||
class Solution { | ||
public: | ||
ListNode* deleteDuplicates(ListNode* head) { | ||
std::map<int, int> cnts; | ||
while (head) { | ||
cnts[head->val]++; | ||
head = head->next; | ||
} | ||
ListNode *deleted_head = NULL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deleted_head は head が削除されているというニュアンスを感じます。 head_of_list_without_duplications であれば、何を表しているか叙述的になると思います。ただ、長いようにも感じます。 head_without_duplications だと、ヘッドに重複がないと誤解される可能性がありますが、ギリギリ大丈夫かと思います。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 前置詞を使った命名を思いつかなかったのでとても新鮮でした。 |
||
for (auto [n, c] : cnts) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 数行程度のスコープであれば 1 文字変数はよいと思うのですが、今回はスコープがやや長いため、叙述的な変数名を付けたほうが良いと思います。それぞれ value と frequency あたりが良いと思います。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ありがとうございます。 |
||
if (c > 1) continue; | ||
if (!head) { | ||
deleted_head = new ListNode(n); | ||
head = deleted_head; | ||
} | ||
else { | ||
head->next = new ListNode(n); | ||
head = head->next; | ||
} | ||
} | ||
return deleted_head; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
#include <set> | ||
|
||
struct ListNode { | ||
int val; | ||
ListNode *next; | ||
ListNode() : val(0), next(nullptr) {} | ||
ListNode(int x) : val(x), next(nullptr) {} | ||
ListNode(int x, ListNode *next) : val(x), next(next) {} | ||
}; | ||
|
||
class Solution { | ||
public: | ||
ListNode* deleteDuplicates(ListNode* head) { | ||
ListNode* seek = head; | ||
ListNode* pre = NULL; | ||
ListNode* border; | ||
int sequence_cnt; | ||
while (seek) { | ||
border = seek; | ||
sequence_cnt = 0; | ||
while (border->next && border->val == border->next->val) { | ||
border = border->next; | ||
sequence_cnt++; | ||
} | ||
if (sequence_cnt == 0) { | ||
pre = seek; | ||
seek = seek->next; | ||
continue; | ||
} | ||
|
||
if (!pre) { | ||
seek = head = border->next; | ||
} | ||
else { | ||
pre->next = border->next; | ||
seek = border->next; | ||
} | ||
} | ||
return head; | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
#include <stdlib.h> | ||
|
||
struct ListNode { | ||
int val; | ||
ListNode *next; | ||
ListNode() : val(0), next(nullptr) {} | ||
ListNode(int x) : val(x), next(nullptr) {} | ||
ListNode(int x, ListNode *next) : val(x), next(next) {} | ||
}; | ||
|
||
class Solution { | ||
public: | ||
ListNode* deleteDuplicates(ListNode* head) { | ||
ListNode* seek = head; | ||
ListNode* pre = NULL; | ||
ListNode* border; | ||
int sequence_cnt; | ||
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. これらの変数がどんな役割を担うのか変数名のみからは分からなかったです。日本語でそれぞれの変数の役割を説明してもらえますか? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
index 0とindex 2ですか?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 読み手に変数の意味を推理させるコードになっているように思います。コメントをつけるか、意味が明らかな変数を使うようにしましょう。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
修正しました。
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ありがとうございます。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 別論点の質問なのですが、今回のように全体のアルゴリズムが少し複雑な場合、変数名(変数の動き)を明確にしても依然全体のアルゴリズムがわかりにくく、困っています。 認知負荷を高めず全体の動きを伝えるために、何か良い伝達方法はありますか。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
L25で
L26で 今回の場合は処理が不要に複雑になっていると思います。自分で上手く説明できない変数を使っているのも一因かと感じました。簡潔な方法を考えてみると良いでしょう。 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 確かに、私の説明はその二つに答えられていません。 その実装はとてもシンプルですね。 アルゴリズムは同じでも、もっとシンプルな構造で書けないか考えます。 |
||
while (seek) { | ||
border = seek; | ||
sequence_cnt = 0; | ||
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. この2つの変数は、while で引き継がないので、ループの中で宣言しましょう。 このコードが読みにくいのは、変数の役割が途中で変わるからです。 pre は取っておくものが見つかるまでは null で見つかったら、見つかったら見つかった最後のものを指します。 pre は、null でまだ見つかっていないものを示す慣例があるからまだいいとしても、この head は日本語で書いても50文字です。これを2つに分けるとまだ読みやすくなります。あとは、関数にすることですね。 それから、他の人のコードをもう少し読む練習をして欲しいです。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 関数にするのはどう考えているかを整理することなので、実際のコードにするかはともかく考えるうえでもしたほうがいいでしょう。 int count_same(ListNode*);
... delete_n(int n, ListNode*); // 返り値は適当に考えてください。消したあとの次でも返しますか
... keep_node(ListNode* node_to_keep, ...); これをしておくと、わりと自然に、完成品の鎖の頭と尻尾が欲しくなるように思います。そしたら変数を用意したらよいでしょう。番兵というテクニックもあります。 それから、不変条件をなんだと思っているのか、つまり、仕事の引き継ぎだと思うと、どういう状態で次のループに引き継いでいるのかを明確にしたほうがいいかもしれません。(繋いでから切断、切断してから繋ぐ、とか。) 取れる選択肢を増やしたほうがよさそうです。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 自然な流れで理解できました。 AtCoderで「whileの中で毎回使う変数は、whileの外で宣言すると宣言時のオーバーヘッドを減らせる」と学んだため、whileの外で宣言していました。 また、関数に切り分けたら実際のコードに残すと思い込んでいたので、考えるための関数化は初めて知りました。 他の人のコードを見て選択肢を増やします。(この問題まで人のコードをほとんど読んでいませんでした。) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
これ、たしかに、コンストラクターが相当に重ければそうでしょうが、これが実際に問題になる状況はちょっと考えにくいです。(宣言時のオーバーヘッドの時間を見積もってみましょう。) AtCoder わりと変な癖がつくので、レートともにだんだん能力が下がるんですよ。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 私の中で、この理由は最近言語化できた気がしています。 状況や問題に対して10個くらい気がつかなくてはいけない点、論点、つまり「ソフトウェアエンジニアリングの常識」があって、そのうちの8,9割が見えていれば、専門家として扱われるんですが、常識のうち、優先順位が低いところしか気が付かない上に、変なプラクティスを覚えるので論点に逆行します。 いや、こういった変なプラクティスは、論点が分かっていない人でも、たまに見かけるはまりどころにはまらないためにあるんで、意味がないわけじゃないんですが、どちらかというと問題は思考をしないように作られているところで、それが積極的に論点を見えなくします。 このあたり、我々、競技プログラミング同好会とその前後の世代が開発したものが結構あって、申し訳ないところなのですが。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ありがとうございます。
痛感しています。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. そうですね。実際に計測してみるのも大事ですが、統計誤差があるので中間出力を比べるのも一つです。 まあ、しかも、最近は、「ただの遊びであることが分かっている」というようになってきましたね。競技プログラミングの先輩たちがそういうからそういうようになってきたんですが、しかし、報酬系の調整までできない水準のメタ認知はむしろ修正が効かなくなるので、ないほうがまし(なのでつけるならば覚悟を持ってつけきらないといけない)という感覚ですね。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ありがとうございます。
報酬系を自力でメタ認知し修正するのは難しいのでしょうね。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
リンクをありがとうございます。 |
||
while (border->next && border->val == border->next->val) { | ||
border = border->next; | ||
sequence_cnt++; | ||
} | ||
if (sequence_cnt == 0) { | ||
pre = seek; | ||
seek = seek->next; | ||
continue; | ||
} | ||
|
||
if (!pre) { | ||
seek = head = border->next; | ||
} | ||
else { | ||
pre->next = border->next; | ||
seek = border->next; | ||
} | ||
Comment on lines
+31
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 一方では「 もし2行に分けるのであれば、 |
||
} | ||
return head; | ||
} | ||
}; |
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.
読む人の認知負荷を下げるため、単語から文字を抜いて略語にするのは避けることをお勧めいたします。
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
また、 key と value がそれぞれ何を表すかを変数名で示すことをお勧めいたします。 value_to_frequency・value_to_frequencies はいかがでしょうか?
Uh oh!
There was an error while loading. Please reload this page.
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.
cnt, tmp, mp, stあたりをよく使ってしまうため、気をつけます。
また、勉強になるリンクをありがとうございます。拝見させていただきます。
とても良いmapの命名ルールをありがとうございます。
確かにそれら命名なら、関数の始まりで定義しても何を格納するか明確に分かります。
また、今回のケースならcountをfrequencyと表現できる点も勉強になりました。
追記:意味的にも
map::count
があることからも、frequencyが良いのですね。