560. Subarray Sm Equals K#15
Conversation
| class Solution { | ||
| public: | ||
| int subarraySum(const std::vector<int>& nums, int k) { | ||
| std::map<int, int> imterim_accumulate_count; |
There was a problem hiding this comment.
imterim は interim の typo でしょうか。
accumulate が動詞のためか、キーと値に何が含まれるか、やや分かりずらく感じました。 prefix_sum_to_frequency や cumulative_sum_to_frequency はいかがでしょうか？
There was a problem hiding this comment.
失礼しました。おっしゃる通りinterimのタイポです…
prefix sumやcumulative sumで累積和の意味があるのを知りませんでした。これらを使ってみます。
| int subarraySum(const std::vector<int>& nums, int k) { | ||
| std::map<int, int> imterim_accumulate_count; | ||
| int ans = 0; | ||
| int accumulate = 0; |
There was a problem hiding this comment.
変数名に動詞の原型を付けるのはあまり見ないように思います。 prefix_sum や cumulative_sum はいかがでしょうか？
| std::map<int, int> imterim_accumulate_count; | ||
| int ans = 0; | ||
| int accumulate = 0; | ||
| for (const auto& num : nums) { |
There was a problem hiding this comment.
参照で受けると、間接アドレッシングで値にアクセスするコードが生成される場合があり、無駄な処理が行われる場合があります。レジスターサイズと比べて大きくない値については、値型で受け取ることをお勧めいたします。
詳しくは「Effective C++ 第3版」の「20項 値渡しよりconst参照渡しを使おう」をご覧ください。 (自分は第 2 版しか読んでおらず、この項の次の項に値渡しを使うことが書かれていました。第 3 版はこの項に値渡しを使う場合のことが書かれているのではないかと予想します。)
こちらもご覧ください。
Ryotaro25/leetcode_first60#44 (comment)
There was a problem hiding this comment.
ありがとうございます。手元にあるので確認してみました。レジスターサイズがどれくらいなのかよくわかっていなかったのですが、基本的に組み込み型は入るのですね。(64-bitPCなら64bit)
| int subarraySum(const std::vector<int>& nums, int k) { | ||
| int result = 0; | ||
| int accumulate = 0; | ||
| std::map<int, int> accumulate_count = {{accumulate, 1}}; |
There was a problem hiding this comment.
個人的にはハッシュマップに名前をつけるときはaccumulate_to_countのように間にtoを入れる方がわかりやすくて好きです。
そして、accumulateは動詞なので、cumulative sumやprefix sumの方がより良さそうです。
There was a problem hiding this comment.
ありがとうございます。たしかにここにaccumulate(動詞)が入るのは不自然な命名ですね。提案頂いた命名も使ってみます。
| result += accumulate_count[accumulate - k]; | ||
| accumulate_count[accumulate]++; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
resultという変数名はあまり情報量がないので、何が入っているのかがわかる名前をつける方が好きです。
今回だと num_subarrays_sum_equals_to_k や num_subarrays でしょうか。
There was a problem hiding this comment.
ありがとうございます。たしかにそもそもメソッド名もあまりわかりやすくないのもあって、途中経過なのかわかりにくい面もありますね。
| int result = 0; | ||
| int accumulate = 0; | ||
| std::map<int, int> accumulate_count = {{accumulate, 1}}; | ||
| for (const auto& num : nums) { |
There was a problem hiding this comment.
intはお大きいデータサイズでは無いのでconst参照にしなくてもいいという意見もございます。
| ### その他 | ||
| 本問にはほぼ関係ないけどunordered_mapの効率化バージョン?であるflat_hash_mapというのがabseil?にあるらしい | ||
| https://hackmd.io/@elkurin/BJgKkoii3 | ||
| Googleの人は標準ライブラリを作ってしまうのですごい… |
There was a problem hiding this comment.
社内の名前は ABC standard library だったかしら。
言語仕様が大きいので、すべてを許容すると管理ができなくなるので一部を切り出して使っています。
There was a problem hiding this comment.
戦略的に作らないと大変そうですね。そう考えると実際に仕様を決めたりgccを開発している人たちはいったい何者なんだ…という気持ちになりました。
ryosuketc
left a comment
ryosuketc
left a comment
There was a problem hiding this comment.
すでにコメントがついている通り、変数名や const 参照に関して見直すとよいかなと思いました。それ以外は特にコメントありませんでした。
6 participants
This Problem
https://leetcode.com/problems/subarray-sum-equals-k/description/