Conversation
nanae772
left a comment
nanae772
left a comment
There was a problem hiding this comment.
お疲れ様です。C++はあまり馴染みが無いのですが、分かりやすいコードでとても読みやすかったです。
少しコメントさせていただきました。参考になれば幸いです。
| } | ||
|
|
||
| std::vector<std::vector<int>> values_by_frequency(nums.size() + 1); | ||
| for (const auto& [val, f] : frequency) { |
There was a problem hiding this comment.
fが一文字変数でぱっと見何を意味するか分からないのでfrequencyなどだと良いなと思ったのですが、frequencyは既にunordered_mapの名前に使われているのですね。
unordered_mapの名前をmemoで書かれているようにnum_frequencyなどにするのはいかがでしょうか？
There was a problem hiding this comment.
@nanae772
こちらご返信が随分と遅くなってしまい申し訳ございません。
コメントありがとうございます。
今回の問題ですが、frequenceyに関連するものが沢山出てきたため、ごちゃごちゃな命名となってしまいました。
後にもご指摘頂く、f, freqなどもいろんな邪念が入ってます。野田さんからもご指摘いただきました。。。
もう少し意図を反映させていきます。
| values_by_frequency[f].push_back(val); | ||
| } | ||
|
|
||
| std::vector<int> answer; |
There was a problem hiding this comment.
answerは実際のところ何を表しているのかがあいまいなので別の名前などだと分かりやすいかなと思いました。
そのまま表すとtop_k_frequent_elementsになりそうですが長すぎるかもしれません。top_kとかで十分かもしれません。
| std::vector<int> answer; | ||
| answer.reserve(k); | ||
| for (int f = static_cast<int>(values_by_frequency.size()) - 1; | ||
| f >= 1 && static_cast<int>(answer.size()) < k; --f) { |
There was a problem hiding this comment.
94行目のif (static_cast<int>(answer.size()) == k) break;があるので、&& static_cast<int>(answer.size()) < kという条件は無くてもよいのかなと思いました。
There was a problem hiding this comment.
@nanae772
ご返信随分と遅くなり申し訳ないです。
コメントありがとうございます！
94行目 break; は 内側ループだけを抜け、外側ループはそのままだと 次の f に進みます。
しかし外側のループ条件に answer.size() < k が入っているので、k 個集めた時点で外側へ再入しない（外側の追加イテレーションを防げる）。
という設計でした。
確かにシンプルに書いた方がいいですね。もう一度書いてみます！
There was a problem hiding this comment.
すみません、二重ループになっていたことを見落としていました 🙇♂️
解説いただきありがとうございます！
| class Solution { | ||
| public: | ||
| std::vector<int> topKFrequent(const std::vector<int>& nums, int k) { | ||
| std::unordered_map<int, int> freq; |
There was a problem hiding this comment.
自分ならキーと値にそれぞれどのようなものが格納されているか分かりやすくするため、 num_to_frequency といった名前を付けると思います。
There was a problem hiding this comment.
@nodchip
こちらご返信が随分と遅くなってしまい申し訳ございません。
コメントありがとうございます。
今回の問題ですが、frequenceyに関連するものが沢山出てきたため、ごちゃごちゃな命名となってしまいました。
後にもご指摘頂く、f, freqなどもいろんな邪念が入ってますね。
もう少し意図を反映させていきます。
| } | ||
|
|
||
| std::vector<std::vector<int>> buckets(nums.size() + 1); | ||
| for (const auto& [val, f] : freq) { |
There was a problem hiding this comment.
関数の引数名が nums のため、 num としたほうが分かりやすいと思います。
| } | ||
|
|
||
| std::vector<std::vector<int>> buckets(nums.size() + 1); | ||
| for (const auto& [val, f] : freq) { |
There was a problem hiding this comment.
f が何を表すのかやや分かりづらく感じました。 frequency とフルスペルで書くと分かりやすくなると思います。
1 文字変数は、 i や s 等インデックスを表したり文字列を表したりすることが想像しやすいもので、かつスコープが短いものであれば、十分わかりやすいと思います。今回はこれらに当てはまらないため、やや分かりづらく感じました。
| * A. バケットソート -> 高頻度側からk個集める。 | ||
| * B. 最小ヒープ（priority_queue） -> (freq, val) を最小ヒープに入れ、サイズが k を超えたら pop。 | ||
| * C. クイックセレクト（選択アルゴリズム） -> (val, freq) の配列（サイズ u）を作り、第 k 大の頻度を基準に分割（平均 O(u)） | ||
| * D. 周波数でソート -> vector<pair<val,freq>> を freq 降順ソートして先頭 k 個。 |
|
既に出ているコメント以外には気になる点はありませんでした。余裕があれば他の解法も実装してみてもいいかもしれません。 |
| - 問題文より、O(nlogn)より良い計算量を目指す。 | ||
|
|
||
| ### 解法を考える | ||
| - バケットソート |
There was a problem hiding this comment.
私が勘違いしていたら申し訳ないですが，今回使われている手法はバケットソートとは別の方法ではないでしょうか．
6 participants
This: https://leetcode.com/problems/top-k-frequent-elements/
Next: https://leetcode.com/problems/find-k-pairs-with-smallest-sums/description/