Conversation
| charIndices[c] = append(charIndices[c], i) | ||
| } | ||
|
|
||
| const NoUniqueChar = -1 |
There was a problem hiding this comment.
const値は関数の先頭 or 外のスコープでまとめて定義するのが一般的かなと思います
| - 最終的に採用して方法は、入力条件に対する汎用性の高い2bの方法 | ||
|
|
||
| ```Go | ||
| const NoUniqueCharacter = -1 |
| for _, c := range s { | ||
| if c < 'a' || c > 'z' { | ||
| log.Fatal("invalid character found in the input") | ||
| panic("unreachable") |
There was a problem hiding this comment.
[質問]
panic("invalid character found in the input")ではだめなんでしょうか？
There was a problem hiding this comment.
Googleのスタイルガイドによると、
Within package main and initialization code, consider log.Exit for errors that should terminate the program (e.g., invalid configuration), as in many of these cases a stack trace will not help the reader. Please note that log.Exit calls os.Exit and any deferred functions will not be run.
For errors that indicate “impossible” conditions, namely bugs that should always be caught during code review and/or testing, a function may reasonably return an error or call log.Fatal.
とあり、panicは基本的には使わず、log.Exitやlog.Fatalを使うようにと記載されています。
There was a problem hiding this comment.
panic vs log.Fatal
自分のためにも簡単に両者についてまとめておきます。
panic: プログラムを止め、関数内と呼び出し側のdeferされたプログラムを実行。トレースを残す。エラーをrecover()によって拾うことが可能。標準エラーに書き出す。
log.Fatal: 標準ライブラリのlog（上のコメント内のlog.Fatalはサードパーティのglog）。log.Printとos.Exit(1)を実行。プログラムを完全停止。ログに書き出す。
| charIndices[c] = append(charIndices[c], i) | ||
| } | ||
|
|
||
| ans := -1 |
There was a problem hiding this comment.
私自身Leetcode等ではたまに書いてしまう気がしますが、業務のコードではansという書き方はあまりしないような気がしました。
少し考えてみたのですが、この関数は問題文と独立に定義できるので、ansというのは問題文ありきで暗黙の文脈があるようです。
この場合、firstUniqueCharIndexとか縮めてfirstIndexとかでもよいかもしれません。
There was a problem hiding this comment.
まだ、result とかのほうがいい気はしますね。「ここに完成品が作られる」っていうことが分かるので。
There was a problem hiding this comment.
コードを読むというのは基本的に「知らない人が読んで理解するというパズル」なんですよね。
その時に多くの場合は上から読んでいくので、どこの情報から出したほうが、早く理解にたどり着くかを考えてあげるということです。
|
|
||
| ```Go | ||
| func firstUniqChar(s string) int { | ||
| charIndices := make(map[rune][]int) |
There was a problem hiding this comment.
細かいですがindexの複数形に合わせてcharsAndIndicesでもいいのかと思いました。
| charIndices[c] = append(charIndices[c], i) | ||
| } | ||
|
|
||
| const NoUniqueChar = -1 |
There was a problem hiding this comment.
この constant は export されないので、名前を lower case で始めた方がいいと思います: https://google.github.io/styleguide/go/decisions#constant-names
|
|
||
| - https://github.com/rihib/leetcode/pull/18 | ||
| - https://github.com/seal-azarashi/leetcode/pull/15 | ||
| - お二方共非常に参考&勉強になった |
| const NoUniqueCharacter = -1 | ||
|
|
||
| func firstUniqChar(s string) int { | ||
| characterFrequency := make(map[rune]int) |
There was a problem hiding this comment.
Goのスタイルガイドには下記のように書かれているので、今回の場合は特に混同するような類似の変数がなく、スコープも小さいことから、単にfrequencyと１語にするのが良さそうです。またkeyがrune型であることはすぐにわかるので、characterという変数名が情報量を追加するかと言われるとそうでもない気がします。
- Single-word names like count or options are a good starting point.
- Additional words can be added to disambiguate similar names, for example userCount and projectCount.
https://google.github.io/styleguide/go/decisions#variable-names
| ``` | ||
|
|
||
| ### Step 3 | ||
| - 最終的に採用して方法は、入力条件に対する汎用性の高い2bの方法 |
There was a problem hiding this comment.
今回の場合、配列だと１次キャッシュに乗り切りますが、mapを使うとメモリ上に散らばるため複数のメモリアクセスが必要になってくるのではないかと思いました。１次キャッシュ参照は0.5nsなのに対して、メモリ参照100nsかかるので、sの長さが10^5だとすると、雑に毎回100nsの差がつくとすると10msぐらいの差がつくのかなと思います。またハッシュのオーバーヘッドや、今回の場合mapの作成時にcapacityを指定していないのでリサイズも複数回起こる気がするのと、TLBヒット率も下がるかなと思うので、もう少し実行時間に差が出るかもしれません。sの長さが10^9ぐらいになると100秒ぐらいの差になると思うので、単に入力条件に対する汎用性だけでなく、パフォーマンスとのトレードオフも考慮に入れて選択するのが良いのではないかと思いました。
There was a problem hiding this comment.
↑すいません、このような見積もりをしてみたのですが、 @oda さん的にはどう思われますでしょうか？（自分の見積もりにあまり自信がないのでご意見を頂けるとありがたいです）
There was a problem hiding this comment.
L1 にも L2 にも乗らないは、サイズだいぶ大きい話に聞こえますね。それぞれ数十k、1M 前後は乗るでしょう。データ構造の都合上、分岐予測のほうが大きな要素かなと推測します。
この下から積み上げる計算は、一番ラフなものであり、気になったらその環境で実験をしてみる、調べてみるなどをするといいでしょう。
それと、大体の場合は速度はあまり優先順位が高くないですね。
7 participants
https://leetcode.com/problems/first-unique-character-in-a-string/description/