929. Unique Email Addresses #28

colorbox · 2024-11-15T14:33:12Z

hayashi-ay/leetcode#25
tshimosake/arai60#2
sakupan102/arai60-practice#15
nittoco/leetcode#7
fhiyo/leetcode#17
TORUS0818/leetcode#16
TORUS0818/leetcode#16
tarinaihitori/leetcode#14
ryoooooory/LeetCode#19
hroc135/leetcode#14
seal-azarashi/leetcode#14
Yoshiki-Iwasa/Arai60#13
kazukiii/leetcode#15
Mike0121/LeetCode#31
SuperHotDogCat/coding-interview#30
Ryotaro25/leetcode_first60#15

oda · 2024-11-15T17:44:36Z

929/step2_3.cpp

+  }
+
+ private:
+  string canonicalize_email(string& email) {


& 関係の質問ですが、意図として、

for (auto email : emails)

ここはコピーして、

string canonicalize_email(string& email)

コピーせずに参照で受け取って、

email.replace(0, at_pos, local_part);

ここで引数を破壊的に変更し、さらにそれをコピーして返す、でいいですか。

速くするためにコピーを減らしたい

numUniqueEmails を呼んだ人が驚かないように壊したくない

は理解できる欲求ですが canonicalize_email の仕様の整合が取れていないように思います。

この書き方でコピーが返されるという点を考慮できていなかったです。

string& canonicalize_email(string& email)

とすべきでした。

はい。それは合理的な選択肢です。

もう一つ、

string canonicalize_email(const string& email)

入力を破壊せずに、変更したものをコピーで返す、というのも選択だと思います。

ここでしたいことは「email から正規化した email を作る」ということですが、結局、使い方を見ると、「破壊していいようなコピーを作る」 + 「破壊的に変更する」という形になっているので、まとめた関数を作るというのも一つです。

他、Google Style Guide の方法だと、「入力を破壊して変更する」際には

void canonicalize_email(string* email);

という signature にする約束になっています。これだと呼び出し元が

canonicalize_email(&email);

と&がつくので、破壊していることが見えるからです。
しかし、これはとてもローカルなルールなので、従う必要はありません。

昔は、string のようなクラスを直接返すと、コピーコンストラクタが走り最適化がされるとも限らなかったのですが、C++11 以降は move も整備されているので、気にしなくてはいけない度合いは下がっているかもしれませんね。
set::insert

他、Google Style Guide の方法だと、「入力を破壊して変更する」際には

補足すると、

非 optional の出力パラメーター、および入出力パラメーターは参照で渡す。

optional の出力パラメーター、および入出力パラメーターは非 const ポインターで渡す。

ようです。

https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs

Non-optional input parameters should usually be values or const references, while non-optional output and input/output parameters should usually be references (which cannot be null). Generally, use std::optional to represent optional by-value inputs, and use a const pointer when the non-optional form would have used a reference. Use non-const pointers to represent optional outputs and optional input/output parameters.

oda · 2024-11-15T17:47:04Z

929/step2_2.cpp

+  string canonicalize_email(string& email) {
+    string canonicalized_email = "";
+    auto at_pos = email.rfind('@');
+    string local_part = email.substr(0, at_pos);


私は、domain 部分も文字列として変数を作ってしまって、replace ではなくて

return canonicalized_email + domain;

としたほうが読みやすいと思います。
要するに名前がつくものにはつけてしまえということです。

なるほど、操作しない箇所には触れないほうが良いと考えていましたが、名前をつけるとわかりやすいですね。

oda · 2024-11-15T17:48:06Z

929/step1.cpp

+Time Order: O(n)
+Space Order: O(n)
+
+方針は早くに定まっていたものの、C++の文字列操作が思うようにいかずに時間がかかってしまった。


まあ、これはそういうものであり慣れですね。定期的にリファレンスを読みましょう。

nodchip · 2024-11-17T13:20:41Z

929/step1.cpp

+      auto at_pos = email.find_first_of("@");
+      string local_part = email.substr(0, at_pos);
+      auto plus_pos = local_part.find_first_of("+");
+      if (plus_pos == std::string::npos) {


string と std::string のうちどちらかに統一したほうが、読むにあたっての認知負荷が下がると思います。

ありがとうございます、nposを使い慣れていなかったのでstd::から書いてしまっていました。

nodchip · 2024-11-17T13:25:24Z

929/step2_1.cpp

+  }
+
+ private:
+  string canonicalize_email(string email) {


関数名は UpperSnake とすることをおすすめいたします。

https://google.github.io/styleguide/cppguide.html#Function_Names

Ordinarily, functions should start with a capital letter and have a capital letter for each new word.

ありがとうございます。
snake caseとcamel caseが混在しているのが良くないですね
leetcodeだとデフォルトで用意されている関数名がcamel caseなので、そちらに合わせるようにします。

nodchip · 2024-11-17T13:35:31Z

929/step2_1.cpp

+  }
+
+ private:
+  string canonicalize_email(string email) {


引数の型のサイズがレジスター幅に比べて十分に大きい場合は、過多なメモリーコピーを避けるため、 const 左辺値参照で渡すことをおすすめいたします。

ありがとうございます、メモリコピーの観点を持てていませんでした。

nodchip · 2024-11-17T13:36:57Z

929/step2_1.cpp

+  }
+
+ private:
+  string canonicalize_email(string email) {


戻り値の型を string にした場合、 RVO (Return Value Optimization) が行われない場合、ヒープメモリーのコピーが発生します。これはプログラムの速度低下を招きます。これについては意識しましたか？

RVOについては初耳でした、コメントありがとうございます。
戻り値の型を気にするようにします。

void canonicalize_email(string& input, string& output)

のようにして、引数のoutputには空の文字列を渡し、outputを書き換える操作をすることで、stringのコピーを明示的に抑えるような方法もあるようです。

nodchip · 2024-11-17T13:37:52Z

929/step2_1.cpp

+
+ private:
+  string canonicalize_email(string email) {
+    const auto at_pos = email.rfind("@");


const を付けている変数と const を付けていない変数がありますが、それらの違いは何でしょうか？意図が分かりませんでした。特に意図がなければ、ローカル変数には const を付けないほうがシンプルに感じられます。意図がある場合は明示的に付けたほうがよいと思います。

コメントありがとうございます。
変更されないものについてはconstをつけ、変更されないものはつけない、と使い分けていました。
constを付与すべき意図が明確な時に限って利用するようにします。

nodchip · 2024-11-17T13:39:00Z

929/step2_2.cpp

+
+ private:
+  string canonicalize_email(string& email) {
+    string canonicalized_email = "";


string のデフォルトコンストラクターで、空文字に初期化されるため、 = "" は消したほうがよいと思います。

nodchip · 2024-11-17T13:44:09Z

929/step2_3.cpp

+  }
+
+ private:
+  string canonicalize_email(string& email) {


他、Google Style Guide の方法だと、「入力を破壊して変更する」際には

補足すると、

非 optional の出力パラメーター、および入出力パラメーターは参照で渡す。

optional の出力パラメーター、および入出力パラメーターは非 const ポインターで渡す。

ようです。

https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs

Non-optional input parameters should usually be values or const references, while non-optional output and input/output parameters should usually be references (which cannot be null). Generally, use std::optional to represent optional by-value inputs, and use a const pointer when the non-optional form would have used a reference. Use non-const pointers to represent optional outputs and optional input/output parameters.

nodchip · 2024-11-17T13:44:59Z

929/step2_3.cpp

+  string canonicalize_email(string& email) {
+    auto at_pos = email.rfind('@');
+    string local_part = email.substr(0, at_pos);
+    std::regex dot_and_plus_suffix("\\.|\\+.*");


正規表現は読むにあたり認知負荷が高く感じます。正規表現の意図をコメントで書くことをおすすめいたします。

なるほど確かに、ありがとうございます、今後正規表現を使用するときは意図を追記します。

solve 929

c5533ee

oda reviewed Nov 15, 2024

View reviewed changes

Fix format

11db03e

nodchip reviewed Nov 17, 2024

View reviewed changes

colorbox added 2 commits November 22, 2024 21:50

fix typo

3ce326c

Add 2_1_fix

6d4ba7a

colorbox merged commit d4ed7ed into main Feb 1, 2025

colorbox deleted the 929 branch February 1, 2025 05:07

929. Unique Email Addresses #28

929. Unique Email Addresses #28

Uh oh!

Conversation

colorbox commented Nov 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants