Conversation
| num_to_freq = defaultdict(int) | ||
| for num in nums: | ||
| num_to_freq[num] += 1 | ||
| heap = [] |
There was a problem hiding this comment.
step1のtop_k_freq_heapとかの方が何が入っているのか分かるので良いかなと思いました。
There was a problem hiding this comment.
ありがとうございます！そうですね, なぜかstep2で命名サボってしまっておりました.
|
|
||
| def swap(l: list, a: int, b: int): | ||
| l[a], l[b] = l[b], l[a] | ||
| return |
There was a problem hiding this comment.
ありがとうございます！末尾にreturnを入れても入れなくても同じ挙動のようですね. それだといれるのは冗長といった感じでしょうか. 今後は外します.
| return store_index | ||
|
|
||
| def quick_select(left: int, right: int, k: int): | ||
| if right <= left + 1: |
There was a problem hiding this comment.
左側にleftが来る方が読みやすいです。
2分探索でも、なんでもいいですが、引数や変数は begin, end と並べたいですね。
left, right でもいいですが。
区間は [left, right) としましょう。そうすると、正常なときには
left <= middle < right です。
ただ、
left >= right
となったら、もう候補がありません。
ここで、
right <= left とかされると混乱します。
There was a problem hiding this comment.
自分も自分で読んでいて頭がこんがらがっていたのですが, 逆にするととても読みやすくなりますね.
ありがとうございます！
|
|
||
| def swap(l: list, a: int, b: int): | ||
| l[a], l[b] = l[b], l[a] | ||
| return |
| quick_select(selected_index + 1, right, k) | ||
| else: | ||
| quick_select(left, selected_index, k) | ||
| return |
| num_to_freq[num] += 1 | ||
| unique_nums = list(num_to_freq.keys()) | ||
|
|
||
| def swap(l: list, a: int, b: int): |
There was a problem hiding this comment.
ローカル関数が多く、コードの見通しが悪いように感じます。メインとなる処理が一か所にまとまっていた方が見やすいです。 class のメンバー変数にしてしまったほうが見通しが良くなると思います。
There was a problem hiding this comment.
ありがとうございます！確かに見にくいですし, topKFrequentと同じレベルで定義されている変数(たとえばnum_to_freq)を勝手に参照してしまうのも気持ち悪いですよね. メソッドとして切り出します.
| if selected_index == k: | ||
| return | ||
| elif selected_index < k: | ||
| quick_select(selected_index + 1, right, k) |
There was a problem hiding this comment.
再帰にせず、ループにした方が見通しが良くなるように感じました。人によって好みが分かれるところかもしれません。
There was a problem hiding this comment.
ありがとうございます！最適化なしの再帰だとスタックが積まれていく分メモリを消費すると思うので, ループのほうがよく感じますね. ループで書き換えます.
3 participants
Problem link