Skip to content

929. Unique Email Addresses#19

Merged
ryoooooory merged 9 commits intomainfrom
task/929
Sep 22, 2024
Merged

929. Unique Email Addresses#19
ryoooooory merged 9 commits intomainfrom
task/929

Conversation

@ryoooooory
Copy link
Copy Markdown
Owner

@Ryotaro25
Copy link
Copy Markdown

調査済みでしたらすみません。
ホスト部に@が来る場合もあるようです。
https://en.wikipedia.org/wiki/Email_address
sakupan102/arai60-practice#15

char c = email.charAt(i);
if (c == '.') {
continue;
} else if (c == '+' || c == '@') {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

好みの問題かもですが、else ifとelseで続けるよりif文だけの方が読みやすいかなと思いました。その場合最後のelse部分はネストが浅くなるので可読性が上がると感じました。

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.

ありがとうございます!
この処理の内容的にメールアドレスを操作していて異常系?をifでまとめていくイメージですので、else if を使うと異常系についてのまとまりをそれぞれ処理、あとはelseで正常系の処理という感じでまとまっていいかなという認識でした。
ただコメントいただいた内容もその通りかなと思いますので、そちらの方でも記述してみます!

}
return uniqueEmails.size();
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ファイルの最後は改行が必要かと思いました。他のファイルも同じようでした。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

レビュー遅くなりましたすみません。

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 on lines +18 to +25
for (int i = email.length() - 1; i >= 0; i--) {
char c = email.charAt(i);
formattedDomain.append(c);
if (c == '@') {
break;
}
}
formattedDomain = formattedDomain.reverse();
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 メソッドで @ 以降の値が取得されていた方が読みやすいのではないかなと思いました。

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.

ありがとうございます!
step2-1と2-2で違う方針をしようと思い、step2-2では練習としてcharを1つずつみるようにしています!

Comment on lines +13 to +15
} else {
formattedEmail.append(c);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

この位置にあるなら else 句にいれる必要はないのかなと思いました。 if, else if どちらかの条件を満たしたらここには到達しないので

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.

僕の認識ですがelseを使わない時は、何も処理をせずに次の処理をする(簡単な処理とかもあるかも)ときのイメージを持っていました。今回はBuilderに加えるという操作もあるのでelseがあると少しだけわかりやすいのかなあともおもってもいます(たしかになくてもいいですし、可読性の観点でもそこまで大きく差はないと思っていますが)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

あ、これは、私は else を消します。理由はインデントが深くなるのと、else が書かれていると下まで目を動かさないといけない場合が増えるんです。

@ryoooooory
Copy link
Copy Markdown
Owner Author

一旦マージしますがコメントなどいつでも追加してください!

@ryoooooory ryoooooory merged commit 75abcf7 into main Sep 22, 2024
@Mike0121
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