Skip to content

Conversation

Comment on lines +15 to +17
if (nums1_set.find(num) != nums1_set.end()) {
continue;
}
Copy link

Choose a reason for hiding this comment

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

この部分、step 3 ではなくてもいいと気がついたみたいですが、信頼できるリファレンスは確認しましたか。
リファレンスに目を通したい気持ちになっておくことは大事かと思います。

Copy link
Owner Author

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

Comment on lines +13 to +17
if (nums1[i] < nums2[j]) {
i++;
} else if (nums2[j] < nums1[i]) {
j++;
} else {
Copy link

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]);
Copy link

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

Copy link

Choose a reason for hiding this comment

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

s/文字/数字/という意味です。

Comment on lines +4 to +7
set<int> unique_nums;
for (int num : nums1) {
unique_nums.insert(num);
}
Copy link

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());

でもいけませんでしたっけ。

Choose a reason for hiding this comment

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

Copy link
Owner Author

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) {
Copy link

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

Copy link
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 1 to 3
/*

*/
Copy link

@seal-azarashi seal-azarashi Nov 5, 2024

Choose a reason for hiding this comment

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

他のファイルを含めてですが、プルリクエスト内でクラスに対するコメントの内容に統一性が無いのが気になりました。多人数で開発をしていると、同僚の実装のコメントが充実していない、理解がしづらいのは結構気になるポイントなので、面接の場で書く実装でもコメントに気を使っておくと良いのではと思います。
例えばレビュワーの私からすると、処理の概要と計算量は必ず書いてあって、解くのにかかった時間はオプション、のようにこのプルリクエストのコメント全てが統一されていると嬉しいなと思います。

Copy link
Owner Author

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());
Copy link

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;

としたほうがシンプルだと思います。

Copy link
Owner Author

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()) {
Copy link

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

Copy link
Owner Author

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よりもシンプルになった
Copy link

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 に共通に含まれているかどうか、という意味を込めてしまっているためだと思います。

Copy link
Owner Author

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などを書く事になってしまっていました。

Comment on lines +9 to +20
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]++;
}

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文は削除できるのかなと思います。

Copy link
Owner Author

@colorbox colorbox Nov 8, 2024

Choose a reason for hiding this comment

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

なるほど、ありがとうございます。
数を数える = カウントアップに気を取られすぎて不要な分岐が増えてしまっていたようです。

}
return intersected_nums;
}
};
Copy link

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を使う必要性が小さいと思いました

Copy link

Choose a reason for hiding this comment

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

と思ったらstep3で実装されていましたね。失礼しました

@colorbox colorbox merged commit 3a4d16b into main Jan 19, 2025
@colorbox colorbox deleted the 349 branch January 19, 2025 14:13
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.

8 participants