Skip to content

347. Top K Frequent Elements#10

Open
hroc135 wants to merge 3 commits intomainfrom
347TopKFrequentElements
Open

347. Top K Frequent Elements#10
hroc135 wants to merge 3 commits intomainfrom
347TopKFrequentElements

Conversation

@hroc135
Copy link
Copy Markdown
Owner

@hroc135 hroc135 commented Jul 31, 2024


### Step 2
- step1をブラッシュアップ
- Goのint型のデフォルト値(nil)が0であることを使い、最初のループの中の発生頻度を数える処理を簡潔にした
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nilはゼロ値とよばれる事もありますが、Goのnilと数値の0は概念的には異なります。
Numeric以外の型のいくつかのデフォルト値がnilというのは正しいですが、例えばint型の変数にnilを代入することはできません。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ご指摘ありがとうございます。理解が曖昧だから言葉にするとボロが出てしまったという項目でした。細かいところではありますが、言語の基礎なので正確に説明できるようにしないとですね

Comment on lines +100 to +102
if h.Len() == k && f <= h.top().freq {
continue
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

読みやすさを優先してこの条件分岐をなくしてしまうのも手だと思いました

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

パフォーマンスクリティカルでない部分のコードだったら確かにそれで良いかもしれませんね

}

h := &numFreqMinHeap{}
for n, f := range numFreqHashMap {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fは一般的に関数を表すことが多いので、freqで良いと思います

heapの要素にもfreqfeldがある紛らわしさからfにしたんだと思いますが、個人的にはfreqにしてもらったほうが読み易いです

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あくまで Go に限った話ですが、Google のスタイルガイドにも次のようにある通り、慣習として一文字にするのは悪くないと個人的には思います。

Abbreviations can be acceptable loop identifiers when the scope is short, for example for _, n := range nodes { ... }.

from: https://google.github.io/styleguide/go/decisions#single-letter-variable-names

ただ f を関数かと推測してしまいそう、というのは確かにそうですね。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

慣習として一文字にするのは悪くないと個人的には思います。

ありがとうございます。Goのスタイルガイドは読んだことありませんでした

たしかにbuffer writerをwにしたりするのはあるあるですね
formatterのインスタンスをfとするのもよく見るかも

ただ、freqfはちょっと違和感かなー 型名とも連動してないし って感想です (^^ ; )

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

たしかに、f は色んなものと被りすぎててちょっと悩ましいです笑

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fが関数を想起させるという点は考えていなかったです。たしかに気をつけた方が良かったですね。


```Go
type numFrequency struct {
n int
Copy link
Copy Markdown

@seal-azarashi seal-azarashi Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

receiver や range では一文字で表しても問題ない (参考) ですが、他のケースではどうかというのは考えてみてもいいと思います。
構造体のフィールド名が一文字にまで略されることは稀に思います。例えば次のスタイルガイドでは、variable の naming についての記述になりますが、次のように書いてあります。

Do not simply drop letters to save typing. For example Sandbox is preferred over Sbx, particularly for exported names.

from: https://google.github.io/styleguide/go/decisions#variable-names

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上記のページ全体を眺めてみましたが、フィールド名が一文字になっていたのはこちらの例ぐらいでした。

type Counter struct {
    m *Metric
}

// from: https://google.github.io/styleguide/go/decisions#receiver-type

これも metricm と命名されるのにならっているだけで、自ら用途を定義しているフィールド名は name といったものになっていますね。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます。numFrequency.numとなると冗長かなと思い、numFrequencyという構造体名からnの意味を推測できるのでこうしておりましたが、goコミュニティの中で一文字フィールド名は稀ということでしたら慣習に従った方が良さそうですね

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants