Conversation
| # @param {Integer} k | ||
| # @return {Integer[]} | ||
| def top_k_frequent(nums, k) | ||
| nums.each_with_object(Hash.new(0)) { |num, num_to_count | num_to_count[num] += 1 } |
There was a problem hiding this comment.
1 行の情報が多く、読みにくく感じました。適宜、どのような状態になったかを端的な英単語・英語句で表した変数に格納したほうが読みやすくなると思います。
| # @param {Integer} k | ||
| # @return {Integer[]} | ||
| def top_k_frequent(nums, k) | ||
| nums.each_with_object(Hash.new(0)) { |num, num_to_count | num_to_count[num] += 1 } |
There was a problem hiding this comment.
Ruby は自分はあまり書かず、あまり流儀が分からないのですが、他の言語では、メソッドチェーンを書くときに、関数呼び出しごとに改行を入れる場合があります。そちらのほうが読みやすくなるかもしれません。
| # @param {Integer} k | ||
| # @return {Integer[]} | ||
| def top_k_frequent(nums, k) | ||
| num_to_freq = nums.each_with_object(Hash.new(0)) { |num, num_to_count | num_to_count[num] += 1 }.to_a |
There was a problem hiding this comment.
英単語から文字を削って識別子とした場合、読み手に取って認知負荷が上がる場合があります。原則フルスペルで書くことをおすすめします。情報科学やソフトウェアエンジニアリングにおいて有名な略語 (API, LAN, DNS, JSON 等) や、所属するチーム内で頻繁に使われている略語は、使用しても問題ないと思います。また、
- number of を表す num_
- sum of を表す sum_
- maximum number of を表す max_
- minimum number of を表す min_
などは、しばしば見かけますので、使ってもよいと思います。
naoto-iwase
left a comment
There was a problem hiding this comment.
お疲れさまです。
3点コメントさせていただきました。
また、省略形を含む変数名、1行の長さについて、nodchip-sanと同様の感想を持ちました。
| # @param {Integer} k | ||
| # @return {Integer[]} | ||
| def top_k_frequent(nums, k) | ||
| nums.each_with_object(Hash.new(0)) { |num, num_to_count | num_to_count[num] += 1 } |
There was a problem hiding this comment.
数え上げは組み込みのメソッドでできるらしいです。
| nums.each_with_object(Hash.new(0)) { |num, num_to_count | num_to_count[num] += 1 } | |
| nums.tally |
https://docs.ruby-lang.org/ja/latest/method/Enumerable/i/tally.html
| end | ||
| end | ||
| result = [] | ||
| while !top_k_heap.empty? |
There was a problem hiding this comment.
while !条件 は、until 条件 と書けるそうです。
| while !top_k_heap.empty? | |
| until top_k_heap.empty? |
https://docs.ruby-lang.org/ja/latest/doc/spec=2fcontrol.html#until
| return nums if k >= nums.size | ||
| num_freqs = nums.each_with_object(Hash.new(0)) { |num, num_to_freq| num_to_freq[num] += 1 } | ||
| .map { |num, freq| NumFreq.new(num, freq) } |
There was a problem hiding this comment.
ここの早期リターンは、numsの重複を取り除いてから判断＋returnしたい気がします。
num_freqs = nums.each_with_object(Hash.new(0)) { |num, num_to_freq| num_to_freq[num] += 1 }
return num_freqs.keys if k >= num_freqs.size| def top_k_frequent(nums, k) | ||
| nums.each_with_object(Hash.new(0)) { |num, num_to_count | num_to_count[num] += 1 } | ||
| .to_a.sort_by { |num, count| [(-1) * count, num ] }.map(&:first)[0...k] | ||
| end |
There was a problem hiding this comment.
お疲れ様です、私も1行の長さが気になりました。
下記くらいの書き方が良いかもしれません（※leetcodeで通ることは確認しましたが、普段rubyは書かないので不適切な点があったらすみません。）
def top_k_frequent(nums, k)
num_to_count = Hash.new(0)
nums.each { |num| num_to_count[num] += 1 }
# [num, count] のペアを頻度降順でソートし、上位k件を取得
num_to_count.sort_by { |_, count| -count }.first(k).map(&:first)
end
解いた問題
347. Top K Frequent Elements
使用言語
Ruby
次に解く問題
373. Find K Pairs with Smallest Sums