Skip to content

Unique Email Addresses step1-3 #15

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

bumbuboon
Copy link
Owner

processed_to_emails = {}
for email in emails:
processed = ""
at_flag = False
Copy link

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
Owner Author

Choose a reason for hiding this comment

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

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

for character in email:
if character == "@":
at_flag = True
if character == "+" and at_flag == False:
Copy link

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
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

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.

if character == "+" and at_flag == False:
plus_flag = True

if at_flag == False:
Copy link

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

local_without_plus = local.split("+")[0]
local_without_plus_and_dot = local_without_plus.replace(".", "")

canonicalized = local_without_plus_and_dot + "@" +domain
Copy link

Choose a reason for hiding this comment

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

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

unique_emails = set()
at = re.compile(r"@")
plus = re.compile(r"\+")
dot = re.compile(r"\.")
Copy link

Choose a reason for hiding this comment

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

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

return len(processed_emails)
```

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

Choose a reason for hiding this comment

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

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

class Solution:
def numUniqueEmails(self, emails: List[str]) -> int:
processed_to_emails = {}
for email in emails:

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

いいとおもいます!

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