Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

387. First Unique Character in a String #29

Merged
merged 2 commits into from
Feb 1, 2025
Merged

387. First Unique Character in a String #29

merged 2 commits into from
Feb 1, 2025

Conversation

Comment on lines +19 to +28
for (int i = 0; i < s.size(); i++) {
if (i < s.size() - 1 && character_indices[i].character == character_indices[i + 1].character) {
count++;
continue;
}
if (count == 1) {
first_unique_character_index = min(first_unique_character_index, character_indices[i].index);
}
count = 1;
}
Copy link

Choose a reason for hiding this comment

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

これちょっと読みにくい気がしますね。
二重ループにするのは一つですよ。えー、こんな感じかな?

for (int i = 0; i < character_indices.size();) {
  int same_end;
  for (same_end = i + 1; same_end < character_indices.size() && character_indices[i].character == character_indices[same_end].character; ++same_end);
  if (same_end - i == 1) {
    first_unique_character_index = min(first_unique_character_index, character_indices[i].index);
  }
  i = same_end;
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど、ありがとうございます。
ループをまたいでcount情報を持つよりもループ内で完結させて二重ループにするほうが読みやすいですね。

for (int i = 0; i < s.size(); i++) {
character_indices.push_back({s[i], i});
}
sort(character_indices.begin(), character_indices.end());
Copy link

Choose a reason for hiding this comment

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

std::ranges::sort を使う手、あと、lambda で何でソートするかを渡す手もあります。

class Solution {
public:
int firstUniqChar(string s) {
LRUCache cache = LRUCache();
Copy link

Choose a reason for hiding this comment

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

LRU という名前は Least Recently Used という意味で、使わないやつから削除するということなので、現在の実装である同じものが2つ以上入ると Remove で linked list だけから削除されるというのは、ちょっと誤解を生む気がします。

出現回数を別の map で管理するか、重複したものを入れておく set を作るかして、このクラスをサイズ無制限の普通の LRU にしておくというのも一つでしょう。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、LRUを改造して今回の問題に対応させましたが、ある程度改造した時点でもうLRUじゃないので名前を変えるなどすべきでした

Comment on lines +27 to +28
const int kNotFound = -1;
return kNotFound;
Copy link

Choose a reason for hiding this comment

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

これ、私はちょっとやりすぎな気がしていて、

return -1;  // Not Found

くらいのコメントでよいのではないでしょうか。
(実際には、.h ファイルにメソッドの説明が書かれると思います。)
あ、つまり、.h にクラスの宣言をして、.cc に実体を書くのが標準的で、こういうイメージです。
https://source.chromium.org/gn/gn/+/main:src/base/files/file.h;l=155?q=%5C%20%5C%20%2F%2F.*-1%20file:.h$

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど、そのあたりの感覚参考になります、ありがとうございます。

public:
int firstUniqChar(string s) {
map<char, int> character_to_count;
for (const auto& c : s) {
Copy link

Choose a reason for hiding this comment

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

型のサイズがレジスター幅以下の場合、 const 参照ではなく値のコピーで受け取ったほうが、処理が速くなると思います。理由は、 const 参照で受け取ると、値を参照する際にアドレスの計算が 1 回入るためです。
for (auto c : s) {
Effective C++ 第3版の「20項 値渡しよりconst参照渡しを使おう」に、例外事項として書いてあったかもしれません。

Copy link
Owner Author

@colorbox colorbox Nov 28, 2024

Choose a reason for hiding this comment

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

コピーが発生しない分、参照が常に有利と考えていましたが、例外があるのですね
ありがとうございます、勉強になります。

Copy link

Choose a reason for hiding this comment

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

現実問題、この場合はどちらも最適化されると思いますが、const 参照にする意図が不明とは感じますね。
リファレンスは、アドレスで実装されることがあるだけであり、概念的には「名前」のようなものともいえるでしょう。(A reference can be thought of as a name of an object. N4860 9.3.3.2 References 第1段落。)

呼出規約を確認ですかねえ。
色々と環境次第なんですが、x64 ABI の場合、大まかに64ビットまではコピーされてレジスターかスタックに積まれ、それ以上はポインターがレジスターかスタックに積まれます。
https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170
https://learn.microsoft.com/ja-jp/cpp/build/x64-calling-convention?view=msvc-170

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど、ありがとうございます、参考にします。

for (const auto& c : s) {
character_to_count[c]++;
}
for (int i = 0; i < s.size() ; i++) {
Copy link

Choose a reason for hiding this comment

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

find_if() 関数を使ったほうがよいかと一瞬思ったのですが、こちらのほうがシンプルだと思います。

}
}

int const kNotFound = -1;
Copy link

Choose a reason for hiding this comment

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

const int kNotFound = -1; のほうがよく見かけるように思います。また、定数には constexpr を優先して使用したほうがよいと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

これは書き方をミスっていました。
ありがとうございます、constexpr勉強になります。

class Solution {
public:
int firstUniqChar(string s) {
map<char, int> character_to_first_index;
Copy link

Choose a reason for hiding this comment

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

1-pass で処理するにあたり、文字をキューに入れていき、 2 回以上出現する文字を取り除いていくというやり方を考えました。

class Solution {
    struct CharacterAndIndex {
        char c;
        int index;
    };
public:
    int firstUniqChar(string s) {
        map<char, int> char_to_count;
        queue<CharacterAndIndex> characters;
        for (int i = 0; i < s.size(); ++i) {
            char c = s[i];
            characters.push({c, i});
            ++char_to_count[c];

            while (!characters.empty() && char_to_count[characters.front().c] >= 2) {
                characters.pop();
            }
        }

        if (!characters.empty()) {
            return characters.front().index;
        }

        return -1;
    }
};

Copy link
Owner Author

Choose a reason for hiding this comment

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

おぉ、なるほど、これは直感的でわかりやすいですね。
ありがとうございます。

Copy link

@hayashi-ay hayashi-ay left a comment

Choose a reason for hiding this comment

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

良いと思います。まあこの問題でそんなに色々別解を試さなくても良いかなという気は個人的にします。

Solve Time: 4:20

Time: O(n)
Space: O(1)

Choose a reason for hiding this comment

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

文字列sの長さがnだとすると、空間計算量もそれに比例して大きくなると思ったのでO(n)になるのかな?と思ったのですが、O(1)になる理由があれば教えていただきたいです!

Copy link
Owner Author

Choose a reason for hiding this comment

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

character_to_countのみ考慮してO(1)って感じです。
引数は考慮していませんでした。

Choose a reason for hiding this comment

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

character_to_countが使うメモリ容量は文字列の長さ(n)に比例して大きくなるのでO(n)かと思ったのですが、どうでしょう?

Copy link
Owner Author

Choose a reason for hiding this comment

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

文字列の長さには比例しません。
文字列に含まれる文字種類数に比例します。

Choose a reason for hiding this comment

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

確かにそうでした!やっと理解できました。ありがとうございます。

Copy link

@philip82148 philip82148 left a comment

Choose a reason for hiding this comment

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

いいと思います!

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