-
Notifications
You must be signed in to change notification settings - Fork 0
349. Intersection of Two Arrays #27
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
Conversation
| if (nums1_set.find(num) != nums1_set.end()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この部分、step 3 ではなくてもいいと気がついたみたいですが、信頼できるリファレンスは確認しましたか。
リファレンスに目を通したい気持ちになっておくことは大事かと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます、無駄なinsertを減らしたくて分岐を書いていたのですが、意味のないコードと気づきやめました。
一応setが一意な オブジェクトを保持するという点は把握していましたが、改めてドキュメントは確認していませんでした。
https://ja.cppreference.com/w/cpp/container/set
| if (nums1[i] < nums2[j]) { | ||
| i++; | ||
| } else if (nums2[j] < nums1[i]) { | ||
| j++; | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私は、if-continue にして最後の else をインデント下げたいです。
| j++; | ||
| } else { | ||
| if (intersected_nums.empty() || intersected_nums.back() != nums1[i]) { | ||
| intersected_nums.push_back(nums1[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私は、ループで同じ文字の間進めちゃうのが好きですが、趣味の範囲でしょう。
https://discord.com/channels/1084280443945353267/1183683738635346001/1188897668827730010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/文字/数字/という意味です。
| set<int> unique_nums; | ||
| for (int num : nums1) { | ||
| unique_nums.insert(num); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
うろ覚えですが、
set<int> unique_nums(nums1.begin(), nums1.end());でもいけませんでしたっけ。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます、その方法で初期化できますし、そちらのほうがシンプルですね。
| @@ -0,0 +1,17 @@ | |||
| class Solution { | |||
| public: | |||
| vector<int> intersection(vector<int>& nums1, vector<int>& nums2) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using namespace std;について。
確かに便利で、LeetCode 練習中に使うなという気はないですが、名前空間を汚すので好まれないということは念頭においておいてください。特にヘッダーファイルでやると include した先が大変です。
Google Style Guide の対応する項目も見ておいてください。
https://google.github.io/styleguide/cppguide.html#Namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます、名前空間については気にしておきます。
| /* | ||
|
|
||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
他のファイルを含めてですが、プルリクエスト内でクラスに対するコメントの内容に統一性が無いのが気になりました。多人数で開発をしていると、同僚の実装のコメントが充実していない、理解がしづらいのは結構気になるポイントなので、面接の場で書く実装でもコメントに気を使っておくと良いのではと思います。
例えばレビュワーの私からすると、処理の概要と計算量は必ず書いてあって、解くのにかかった時間はオプション、のようにこのプルリクエストのコメント全てが統一されていると嬉しいなと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど、ちょっとコメントを書き忘れた状態でコミットしてしまっていたようなので以後気にします、
| class Solution { | ||
| public: | ||
| vector<int> intersection(vector<int>& nums1, vector<int>& nums2) { | ||
| sort(nums1.begin(), nums1.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set<int> set1(nums1.begin(), nums1.end());
set<int> set2(nums2.begin(), nums2.end());
vector<int> intersected;
set_intersection(set1.begin(), set1.end(), set2.begin(), set2.end(), back_inserter(intersected));
return intersected;
としたほうがシンプルだと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setをそのままset_intersectionに利用できないと勘違いしていました。
こちらのほうがシンプルですね、ありがとうございます。
| vector<int> intersection(vector<int>& nums1, vector<int>& nums2) { | ||
| set<int> nums1_set; | ||
| for (int num : nums1) { | ||
| if (nums1_set.find(num) != nums1_set.end()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::set::contains() のほうがシンプルになると思います。
https://cpprefjp.github.io/reference/set/set/contains.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます、こちらのほうがよいですね。
349/step2_1.cpp
Outdated
| @@ -0,0 +1,29 @@ | |||
| /* | |||
| mapで書き直したパターン | |||
| 単一のmapでカウントできるのでsetを利用するstepqよりもシンプルになった | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ロジックがややパズルになっているように感じます。マジックナンバーの 1 に nums1 に含まれているかどうか、マジックナンバーの 2 に nums1 と nums2 に共通に含まれているかどうか、という意味を込めてしまっているためだと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。
https://github.com/colorbox/leetcode/pull/27/files#r1830993992 でも指摘されましたが、カウントアップの利用にこだわった結果不要なcontinueなどを書く事になってしまっていました。
| for (int num : nums1) { | ||
| if (nums_count[num] == 1) { | ||
| continue; | ||
| } | ||
| nums_count[num]++; | ||
| } | ||
| for (int num : nums2) { | ||
| if (nums_count[num] != 1) { | ||
| continue; | ||
| } | ||
| nums_count[num]++; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9-14行目でnums1の各要素が出現したら、値に1を設定し、
15-20行目のnums2 の要素が nums1 にも存在する場合のみカウントを2にすれば、
10行目のif文は削除できるのかなと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど、ありがとうございます。
数を数える = カウントアップに気を取られすぎて不要な分岐が増えてしまっていたようです。
| } | ||
| return intersected_nums; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このアルゴリズムはnums1に含まれる要素をリストアップした後、nums2を舐めてnums1にも同じ要素がないか参照し、あればintersected_nums(答え)に追加する、ということだと思いますが、それだとnums1の要素をmapで管理する必要はなく、setでいいかと思います。つまり、num_to_countのキーには意味がありますが、値に意味はないのでmapを使う必要性が小さいと思いました
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
と思ったらstep3で実装されていましたね。失礼しました
349. Intersection of Two Arrays
https://discord.com/channels/1084280443945353267/1183683738635346001/1188146024565444689
hayashi-ay/leetcode#21
https://discord.com/channels/1084280443945353267/1196472827457589338/1196541121912909824
tshimosake/arai60#3
sakupan102/arai60-practice#14
nittoco/leetcode#15
fhiyo/leetcode#16
TORUS0818/leetcode#15
Ryotaro25/leetcode_first60#14
Mike0121/LeetCode#30
kazukiii/leetcode#14
Yoshiki-Iwasa/Arai60#12
SuperHotDogCat/coding-interview#33
seal-azarashi/leetcode#13
hroc135/leetcode#13
tarinaihitori/leetcode#13
shining-ai/leetcode#13