Skip to content

387. First Unique Character in a String#15

Open
seal-azarashi wants to merge 12 commits into
mainfrom
first-unique-character-in-a-string
Open

387. First Unique Character in a String#15
seal-azarashi wants to merge 12 commits into
mainfrom
first-unique-character-in-a-string

Conversation

@seal-azarashi
Copy link
Copy Markdown
Owner

Copy link
Copy Markdown

@Yoshiki-Iwasa Yoshiki-Iwasa left a comment

Choose a reason for hiding this comment

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

良いと思います

実装による速度変化の考察も丁寧で自分もこのくらい丁寧に取り組みたいと思いました

// 空間計算量: O(n)
class Solution {
public int firstUniqChar(String s) {
// サロゲートペアや合成文字等、char の値を複数用いないと表現出来ない文字が来ないことを想定しています。
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

Choose a reason for hiding this comment

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

Java は内部では UTF-16 使っているんでしたね。

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.

Java は内部では UTF-16 使っているんでしたね。

はい、そうです。

英語が主言語の外資系企業でこの点を考慮する価値がどこまであるのかはわかりませんが、サロゲートペアの対応は日本語対応の web アプリケーション作る限りは避けられない問題ですので、一応コメントしておきました。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

最近は絵文字のおかげでいるんですよね。

https://note.com/ttuusskk/n/n1bff5d8e638c

普段はGoogleでAndroidのTextまわりの開発を行っており、DroidKaigiやShibuya APKで発表させていただいたりしています。最近はほぼ絵文字の話しかしてないので、絵文字おじさんと思われてそうですが、普段の仕事は絵文字に限らず、Androidの文字表示の部分は大抵面倒をみています。

絵文字は比較的新しい文字なので、ほとんどはこのBMPではない領域に収録されています。ですので、絵文字を処理するということは、ほぼサロゲートペアの処理が必要になると思ってください。

Copy link
Copy Markdown
Owner Author

@seal-azarashi seal-azarashi Aug 8, 2024

Choose a reason for hiding this comment

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

ああ、確かに絵文字はどの言語圏でも使うので 16bit を超えた際の考慮は必要ですね。ご指摘ありがとうございます。

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.

そして引用頂いた記事、とてもおもしろかったです!絵文字関連の仕事をされている方々には頭が上がらない思いです😂

グラフィム判定のように、ランダムな場所で「ここはグラフィム境界ですか?」という質問に応えようと思うと、指定された場所だけでは判定ができず、遡って最初まで行き、指定された場所から前に偶数個のRIがあった場合はグラフィム境界、奇数個のRIがあった場合はグラフィム境界ではない、という判定をせざるを得ません。これはつまり、最悪ケースで指定の位置がグラフィム境界であるかを判定するのにすべてのテキストを走査する必要があることを意味します。

なんでサロゲートペアみたいに1文字目と2文字目でグループを分けなかったのかとか、なんで間にZWJ入れなかったのかとか色々と後出しジャンケンはできますが、残念ながら今から変えることはできないので、ぐぬぬと言いながら実装しましょう。

こことかめっちゃつらいですね...😇

- 単純になったので多分 JVM の JIT コンパイラにとってもより最適化しやすくなっているかもしれない
- そういえば空間計算量は O(1) に改善されるので、最初からこちらを選んでもよかったなと振り返って思う
- 一方で lower case english letter 以外は扱えない、ユースケースを想像すると中々ピーキーな実装になるので、面接官に対しては、過度なチューニングをいきなりし始めるような印象を与えないように気をつけたい
- 定数を static にする利点がほぼないので、この修飾子を外した
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

逆にこの定数をインスタンスごとに持つ利点ってあるんでしょうか?

あと、NOT_FOUNDは使う側でも参照する可能性がまあまあある気がするので、staticでpublicなメンバにして Solution.NOT_FOUNDという形で他から使えるようにしても良いかもと思いました。

Copy link
Copy Markdown
Owner Author

@seal-azarashi seal-azarashi Aug 6, 2024

Choose a reason for hiding this comment

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

逆にこの定数をインスタンスごとに持つ利点ってあるんでしょうか?

いえ、特に無いと思います。一方で static メンバにしても、全てのインスタンスで同じ値が参照されることで得られる以下のメリットを享受できないので、記述ないし設定を減らして読みやすくするためにこの修飾子を消してしまうのがベターかなと考えました。

  • メモリ消費量が少なくなる
    • しかし今回は微々たる容量しかなく、また Leetcode 上ではインスタンスが複数作成されることは考えづらい
  • 値を変更する際、その処理が一度で済む (各インスタンスそれぞれのメンバに対して実施する必要がない)
  • ユーティリティクラスやシングルトンの作成に活用出来る

NOT_FOUNDは使う側でも参照する可能性がまあまあある気がするので、staticでpublicなメンバにして Solution.NOT_FOUNDという形で他から使えるようにしても良いかもと思いました。

確かにそうですね!Step 4 で修正しました: 389f43d

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.

しかし今回のように「なぜ static にしないのか?」とコードを読む人が疑問に思う可能性もあるので、パフォーマンス等々は置いておいて、static にしておいてもいいのかなと改めて思いました。しない理由も特にないので...

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.

ということで全て static にしました: 1c4baab

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

記述ないし設定を減らして読みやすくするためにこの修飾子を消してしまうのがベター

個人的にはstatic修飾子の有無で読みやすさはあまり変わらない気がしており、またSolutionインスタンスが大量に生成されるケース (中々無いとは思いますが、クライアント側の問題なのでハンドルできない以上可能性としてあり得る話ではある) にほぼノーコストで対応できるので、どちらかというとstaticを付けない選択をする方に疑問を持った感じでした。

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.

そうですね。不要なものは無いに越したことはない、ぐらいに思ってましたが、static があった程度で読みづらくなるということはないですし、仰るとおりインスタンスが大量に作成される可能性はありますので、今回は static を付けるのが最適な選択だと思いました。ご指摘ありがとうございます。

Copy link
Copy Markdown

@kazukiii kazukiii left a comment

Choose a reason for hiding this comment

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

良いと思います!

- 【🚨レビュワーの皆様、この点何か思いつくものなどありましたらご指摘頂けると嬉しいです】この処理が早い要因としては次のようなものが考えられるだろうか:
- 要素数26の配列である alphabetFrequency はオーバーヘッドを含めなければ100バイト程度の大きさしかないので、これを扱う際には、L1 データキャッシュが活用されていると思われる
- ハッシュ計算、ハッシュ衝突時の走査といった処理がなくなり、代わりにずっと単純な `s.charAt(i) - 'a'` が実行されるようになった
- 単純になったので多分 JVM の JIT コンパイラにとってもより最適化しやすくなっているかもしれない
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Javaはあまり詳しくないですが、boxing/unboxingによるオーバーヘッドも考えられそうです。

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.

ありがとうございます!確かにそうですね。追記させて頂きました: 5f0a953

@TORUS0818
Copy link
Copy Markdown

拝見しました。
コメントで議論されている部分以外は良いと思いました。

あと、計算量の観点から実装を見送られているものがいくつかありましたが、(余力があれば)色々なバリエーションの解答を見れるといいなと個人的には思います。

@seal-azarashi
Copy link
Copy Markdown
Owner Author

拝見しました。 コメントで議論されている部分以外は良いと思いました。

あと、計算量の観点から実装を見送られているものがいくつかありましたが、(余力があれば)色々なバリエーションの解答を見れるといいなと個人的には思います。

ありがとうございます。自分の引き出しを増やすためにも、今後は積極的に書いていこうと思います。

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.

6 participants