Conversation
| if (!emails[i].contains("@")) { | ||
| continue; | ||
| } | ||
| String localName = emails[i].replaceFirst("@.*$", ""); |
There was a problem hiding this comment.
正規表現は読み手にとってやや認知負荷が高いかもしれません。この程度の複雑さのものであれば大丈夫だと思いますが、もう少し複雑なものはコメントで内容を補足するとよいと思います。
| if (!email.contains("@")) { | ||
| continue; | ||
| } | ||
| String address[] = email.split("(?=@[^@]*$)", 2); |
There was a problem hiding this comment.
この正規表現は、ぱっと見で何をやっているのか分かりにくく感じました。コメントで内容を補足するか、平易なコードに書き換えるとよいと思いました。
余談ですが、小田さんの「ソフトウェアエンジニアリングは複雑なことをすることではない。」という言葉を思い出しました。
There was a problem hiding this comment.
個人的に正規表現が以前からパズル感覚で好きでして、つい正規表現頼みで書いてしまっていました。
ご指摘いただいた通りで、
自分でも、このあたりの正規表現の書き間違いが原因で、正しい結果が返ってこないことが何度かありました。
こだわりや好みではなく、目的に照らして何が適切な方法かを考慮するところがエンジニアとしては足りていなかったのかなと思います。
コメントなども適宜使うようにしようと思います。
レビューいただき、ありがとうございます。
| @@ -0,0 +1,176 @@ | |||
| ##STEP1 | |||
There was a problem hiding this comment.
本筋とは関係ありませんが、.md ファイルならマークダウンの記法に則ると良いと思います。例えばGitHubではレンダリングした結果も見えます。
| ##STEP1 | |
| ## STEP1 |
There was a problem hiding this comment.
のあとにスペースがいるんですね！
マークダウンの記法を調べて書いてみたんですが、
見出しになっている感じがなくて変だと思っていました…。
レビューありがとうございます。
There was a problem hiding this comment.
すみません、「##」のあとにスペースを入れたら
当たり前ですが普通にマークダウンとしての表記になり、
文がおかしくなってしまいました笑
色々調べてやってみます。
| ・メールアドレスの長さは最大254字→正規化後の長さのチェックが必要 | ||
| ・ローカルネームの先頭は+ではいけない | ||
| ・ドメインネームは.comで終わる | ||
| 不正なメールアドレスが来たときは、continueして次のアドレスの処理をすることとする。 |
There was a problem hiding this comment.
不正なメールアドレスが来たときは、continueして次のアドレスの処理をすることとする。
これが最適な場面やユースケースが何か、深堀りしてみると良いと思いました。
There was a problem hiding this comment.
本格的な開発プロジェクトにいたことがなく、
表現として合っているのか分からないのですが、
バッチ処理で複数件のメールアドレスを処理する場面を考えていました。
オンラインで、単件の入力をチェックするという場面であれば、不正なメールアドレスに対してエラー処理をするのがいいのかなと思います。
There was a problem hiding this comment.
バッチ処理で複数件のメールアドレスを処理する場面
この「処理」がより噛み砕けるとよりよいです。
参考までに過去のコメントを引用しておきます。
Yoshiki-Iwasa/Arai60#13 (comment)
たとえば、これ、マーケティングのメールを送りたいのか、ある集団のやりとりを整理したいのか。
つまり、たとえば、誤った入力が一つ入ったときに、全体として、そこそこ動いて欲しいのか、異常だといって止まって欲しいのか。それは何をしたいのかとの兼ね合いになるでしょう。
syoshida20/leetcode#20 (comment)
これ、たとえば、データサイエンス目的の研究で、バッチで統計処理をしているならば、むしろ落ちて欲しいんですよね。
There was a problem hiding this comment.
参考のご提示ありがとうございます。
データサイエンスや業務的な処理などいろいろあり得そうですね。
そこまで深く思いつかなかったので、勉強になります。
実務経験が偏っており、イメージできるユースケースの解像度があまり高くない状態なのかなと思います。
解像度の低さをどうすれば解消できるのか、いまいまはわからないのですが、
とりあえずは、課題として言語化できたのでよかったです。
コメントいただきありがとうございます。
| continue; | ||
| } | ||
| String splitAddress[] = email.split("(?=@[^@]*$)", 2); | ||
| splitAddress[0] = splitAddress[0].replaceAll("\\+.*$", ""); |
There was a problem hiding this comment.
Javaのお作法に沿っているかわかりませんが、local_name とか domain_name という変数に束縛して操作したほうが、読みやすいですし、書いているときのミスも防止できそうです。
There was a problem hiding this comment.
Javaの作法を私もあまり詳しくないのですが、
変数を作ったほうがいいというのは、ご指摘の通りだと思います。今回の問題でインデックスの指定の間違いで、正しい結果が返ってこないことが何度かありました。
step1ではlocalNameという変数を宣言して書いたのですが、localNameとしては2、3行しか使わなかったので、step2、3では変数を使わないで書きました。（が、すでに書き間違いを何度かしました。）
書いているときにミスをしてしまったから
そこで書き方を工夫するとか、
読みやすさの点について、今は考慮が足りていないと思いました。精進します。
ちなみに、javaの変数名の作法としては
キャメルケースが一般的だそうです。
レビューありがとうございます。
|
時間がかかる問題ですが、積極的に答えを見ていっていいと思いますよ。写経だけでも別にいいのではないだろうか、くらいに思っています。 |
TakayaShirai
left a comment
TakayaShirai
left a comment
There was a problem hiding this comment.
指摘されている部分以外は読みやすかったと思います！正規表現はパズル的な側面が大きいので、個人的にはコメントがついているとより読みやすいです！
5 participants
今回の問題
929. Unique Email Addresses
次の問題
141. Linked List Cycle
一問取り組むのに1か月近くかかってしまっているので、
先にArai60の各カテゴリのEasyの問題を解き進めていく方針で考えています。
HashMapのEasyは一通り解いたので、次はLinkedListに着手しようと思います。