Skip to content

Unique Email Addresses step1-3#15

Open
bumbuboon wants to merge 1 commit intomainfrom
uniquEmailAddresses
Open

Unique Email Addresses step1-3#15
bumbuboon wants to merge 1 commit intomainfrom
uniquEmailAddresses

Conversation

@bumbuboon
Copy link
Copy Markdown
Owner

Comment thread uniqueEmailAddresses.md
processed_to_emails = {}
for email in emails:
processed = ""
at_flag = False
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

flag という単語は、変数名に対して意味を追加していないように思います。 @ が見つかったかどうかを表す変数ですので、 found_at_sign はいかがでしょうか? plus_flag も found_plus あたりが良いと思います。

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.

ありがとうございます。
最初に書いた時点では良い名前が思いついていなかったです。

Comment thread uniqueEmailAddresses.md
for character in email:
if character == "@":
at_flag = True
if character == "+" and at_flag == False:
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 at_flag が良いと思います。

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.

それはどうしてでしょうか?
at_flagはTrue, False以外になる可能性がないので not at_flagで十分ということでしょうか。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

== 演算子の戻り値が bool 値であり、冗長に感じられるためです。以下のサイトも参考になるかと思います。

https://peps.python.org/pep-0008/#programming-recommendations

Don’t compare boolean values to True or False using ==:

https://google.github.io/styleguide/pyguide.html#2144-decision

Never compare a boolean variable to False using ==. Use if not x: instead.

Comment thread uniqueEmailAddresses.md
if character == "+" and at_flag == False:
plus_flag = True

if at_flag == False:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if 文のネストと else 文の構成が読みづらく感じました。以下のように early return をしてはいかがでしょうか?

if at_lag:
    processed += character
    continue

if not plus_flag and character != ".":
    processed += character

Comment thread uniqueEmailAddresses.md
local_without_plus = local.split("+")[0]
local_without_plus_and_dot = local_without_plus.replace(".", "")

canonicalized = local_without_plus_and_dot + "@" +domain
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: + のあとにスペースを空けることをお勧めいたします。

Comment thread uniqueEmailAddresses.md
unique_emails = set()
at = re.compile(r"@")
plus = re.compile(r"\+")
dot = re.compile(r"\.")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

タブとスペースが混在しているようです。

Comment thread uniqueEmailAddresses.md
return len(processed_emails)
```

下のようにしたら動くが、無理やり感が強い。良い方法はないか。
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

下に出てくる split が一つの解ですね。
見慣れない関数を見たら docs.python などに目を通しておきましょう。

Comment thread uniqueEmailAddresses.md
class Solution:
def numUniqueEmails(self, emails: List[str]) -> int:
processed_to_emails = {}
for email in emails:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

emailのcanonicalizeで結構複雑なことをしているので、メソッドに切り出すとネストが浅くなる + 切り出した処理にメソッド名という名前をつけれる、などの理由から読みやすくなりそうです。

@ryoooooory
Copy link
Copy Markdown

いいとおもいます!

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.

5 participants