929. Unique Email Addresses #16

TORUS0818 · 2024-06-12T07:13:15Z

https://leetcode.com/problems/unique-email-addresses/description/

goto-untrapped · 2024-06-12T14:28:06Z

easy/929/answer.md

+        def normalize(email: str) -> str:
+            is_local_part = True
+            is_ignore_part = False
+            elements_of_normalization = []


他の単語がシンプルな分、elements_of_normalization という名前に違和感がありました。（長いな、何が入っているのかパッと分からなかったな）normalized_chars のような単語もありなのかなと思いました。

有難うございます！
確かに他の変数名とのバランスも考えた方がいいかもしれませんね。

英語としても不自然に聞こえます。

有難うございます。
あまり感覚がないので矯正していきたいです。

goto-untrapped · 2024-06-12T14:32:44Z

easy/929/answer.md

+            is_local_part = True
+            is_ignore_part = False
+            elements_of_normalization = []
+            for char in email:


予約語になり得る単語を変数名として使うのはよろしくないというコメントを、どこかで見た気がします。

一応pythonでの予約語にはなっていない筈ですが、紛らわしいかもしれませんね。

liquo-rice · 2024-06-12T17:53:28Z

easy/929/answer.md

+            normalized_local_part = local_part.split('+')[0]
+            normalized_local_part = normalized_local_part.replace('.', '')
+
+            return f'{normalized_local_part}@{domain_part}'


stringを作らないで、tupleで返してもよさそうです。

確かに正規化されたメールアドレスを作る必要は必ずしもないですね。
有難うございます。

liquo-rice · 2024-06-12T17:54:54Z

easy/929/answer.md

+            local = email.split('@')[0]
+            domain = email.split('@')[1]
+            if local.find('+') > 0:
+                local = local[:local.find('+')]


２回local.find('+')をしているのが気になります。

そうですね。
1回インデックスを取得して使いまわせば良かったです。

liquo-rice · 2024-06-12T17:57:31Z

easy/929/answer.md

+        def normalize(email: str) -> str:
+            is_local_part = True
+            is_ignore_part = False
+            elements_of_normalization = []


英語としても不自然に聞こえます。

liquo-rice · 2024-06-12T17:57:57Z

easy/929/answer.md

+    def numUniqueEmails(self, emails: List[str]) -> int:
+        def normalize(email: str) -> str:
+            is_local_part = True
+            is_ignore_part = False


is_ignored_part
複数のループに分けた方が自然だと思います。

fhiyoさんのコメントにもありましたね。
実装し直してみようと思います。

無駄なfindをしないように修正

パート毎に分けてループ処理する方式の実装を追加

oda · 2024-06-13T17:56:43Z

easy/929/answer.md

+      - https://www.ietf.org/rfc/rfc1034.txt#:~:text=The%20labels%20must%20follow%20the%20rules%20for%20ARPANET%20host%20names.%20%20They%20must%0Astart%20with%20a%20letter%2C%20end%20with%20a%20letter%20or%20digit%2C%20and%20have%20as%20interior%0Acharacters%20only%20letters%2C%20digits%2C%20and%20hyphen.%20%20There%20are%20also%20some%0Arestrictions%20on%20the%20length.%20%20Labels%20must%20be%2063%20characters%20or%20less
+    - RFC5322
+      - https://datatracker.ietf.org/doc/html/rfc5322#:~:text=local-part%20%20%20%20%20%20%3D%20%20%20dot-atom%20/%20quoted-string%20/%20obs-local-part
+


エンジニアというのはこういうところを普段から読んでいるので、「普段だったらリファレンスマニュアルや RFC を確認する」という発言は好意的に受け取られます。

TORUS0818 · 2024-06-17T04:37:53Z

liquo-riceさん、goto-untrappedさん、odaさん、レビューをありがとうございました！

Yoshiki-Iwasa · 2024-06-23T00:44:10Z

easy/929/answer.md

+    > プログラムが停止する。  
+    > エラーを示す値を返す。  


@TORUS0818
”プログラムが停止する”の選択肢の場合って、何を返す想定でしょうか？？

例えば

email = '"@"@.com' local_name, domain_name = email.split('@')

とかでValueError: too many values to unpack (expected 2)みたいな感じでしょうか。

log Fatal を想定しています。よい並列計算ミドルウェアがあったりするとこれがよいケースがあります。

ミスリードしてました。

@TORUS0818
ありがとうございます。

ValueError: too many values to unpack (expected 2)

これで落ちちゃうとログを見に来た人は想定された終了なのか、意図しない異常終了なのか分からなくて焦ると思うので、親切なlog残して落ちて上げたほうがいいのかなと思いました!

@oda
ありがとうございます。

よい並列計算ミドルウェアがあったりするとこれがよいケースがあります

すいません。なぜここで並列計算ミドルウェアの話が出てくるんでしょうか（？？　）

ありがとうございます！
ここら辺の感覚、経験不足であまりないので意識していきたいと思います。

現代のプログラム

コードを書くときには、エンジニアリングをしてくださいと言っていますが、そもそもそのプログラムが何のために動いているかと、細かい動作をどうしたいかを接続することもそのエンジニアリングです。要は、大局観をもって行動して欲しいということです。それで、現代のプログラムは、人が起動して一つだけで動いている場合だけではないので、そうではない想定を考えてよしあしを決めてください。

ありがとうございます。

実は現在の職場では（暗に）その真逆を求められています。。
書いたコードは基本他人は読まないし、責任は自分で持つ、細かいことに拘っている時間があるならその分早く結果（なんらかの数値や動くもの、という意味です）を出したほうが価値がある、という感じです。
（もちろん、もし同じスピードで、そのような配慮がなされたコードが書けるのならその方が良い、というのは合意していただけるとは思うのですが）

元々、上記のような状況に課題を感じて意識的に修正しようとはしていたのですが、まだまだ見えていないものが多いと感じます。皆さんと積極的にコミニュケーションして、この練習会でしっかり身につけられるよう頑張ります！

seal-azarashi · 2024-07-13T14:32:00Z

easy/929/answer.md

+    def numUniqueEmails(self, emails: List[str]) -> int:
+        unique_email_address = set()
+        for email in emails:
+            local_part, domain_part = email.rsplit('@', maxsplit=1)


Constraints に "Each emails[i] contains exactly one '@' character." とあるので、第二引数は書かなくても良いかもしれません。

ここはこんな背景があります。
https://discord.com/channels/1084280443945353267/1225849404037009609/1253042734252556391

seal-azarashi · 2024-07-13T14:34:15Z

easy/929/answer.md

+            local_part, domain_part = email.rsplit('@', maxsplit=1)
+            normalized_local_part = local_part.split('+')[0]
+            normalized_local_part = normalized_local_part.replace('.', '')


Python の文字列型は immutable なので、normalized_local_part を作るのに3つ値が宣言されますね。これを1回で済むように改善してもいいかなと思いました。

有難うございます。
具体的にはどのようにすれば良さそうでしょうか。

他の方に同じコメントをしたら oda さんから返信がありました。ちゃんと見て後ほど返信しますが、仰られてる通りあまり適切でないレビューだった気がしています... すいません🙏
SuperHotDogCat/coding-interview#30 (comment)

有難うございます。
具体的にはどのようにすれば良さそうでしょうか。

上記 oda さんのコメントにもある通りおそらく遅くなってしまいますが... 以下のようにすればオブジェクトの生成数は減らせるかと思います

local_part, domain_part = email.rsplit('@', maxsplit=1) normalized_local_part = [] for char in local_part: if char == '+': break if char != '.': normalized_local_part.append(char)

Create answer.md

21ca3d3

goto-untrapped reviewed Jun 12, 2024

View reviewed changes

liquo-rice reviewed Jun 12, 2024

View reviewed changes

TORUS0818 added 2 commits June 13, 2024 10:58

Update answer.md

565d265

無駄なfindをしないように修正

Update answer.md

0cfd18a

パート毎に分けてループ処理する方式の実装を追加

oda reviewed Jun 13, 2024

View reviewed changes

Yoshiki-Iwasa reviewed Jun 23, 2024

View reviewed changes

seal-azarashi reviewed Jul 13, 2024

View reviewed changes

colorbox mentioned this pull request Nov 15, 2024

929. Unique Email Addresses colorbox/leetcode#28

Merged

		> プログラムが停止する。
		> エラーを示す値を返す。

929. Unique Email Addresses #16

Are you sure you want to change the base?

929. Unique Email Addresses #16

Uh oh!

Conversation

TORUS0818 commented Jun 12, 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!

TORUS0818 commented Jun 17, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants