Skip to content

Conversation

class Solution {
public:
vector<vector<int>> kSmallestPairs(vector<int>& nums1, vector<int>& nums2, int k) {
long long int left = nums1.front() + nums2.front();
Copy link

Choose a reason for hiding this comment

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

整数は 2 つまでしか足されず、 -109 <= num <= 109 のため、 int の範囲を超えることはありません。よって int で十分だと思います。また、 long long int はあまり見かけません。古いコードだと long long、比較的新しいコードだと、ビット幅が決まっている int64_t を使うように思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、こちらintで十分ですね。

int64_t勉強になります。
https://cpprefjp.github.io/reference/cstdint/int64_t.html

continue;
}
int count = it - nums2.begin();
count_sum += count;
Copy link

Choose a reason for hiding this comment

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

このあとには

if (count_sum > limit) {
  return limit;
}

はいらないのでしょうか?

Copy link

Choose a reason for hiding this comment

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

たぶん、これ結果的には動きますね。二分探索で、limit 以上の部分と未満の部分の境界を探すという動きをしているので。

Copy link
Owner Author

Choose a reason for hiding this comment

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

これは既存のif節を消す、もしくはこの行に

if (count_sum > limit) {
  return limit;
}

を追加するほうが挙動が一貫するので良いですね
今の実装だと挙動が一貫していないのでよくなさそうです

int count_sum = 0;
for (int num1 : nums1) {
auto it = upper_bound(nums2.begin(), nums2.end(), num - num1);
if (it == nums2.end()) {
Copy link

Choose a reason for hiding this comment

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

どちらのケースも
count_sum += std::distance(nums2.begin(), it);
と書けると思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、これはdistanceを使用して分岐をなくすほうがシンプルですね。

};

int count_less_pairs(vector<int> nums1, vector<int>nums2, int num, int limit) {
int count_sum = 0;
Copy link

Choose a reason for hiding this comment

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

count_sum という変数名は count が動詞のように見え、関数名のように感じられました。 num_pairs はいかがでしょうか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、確かに動詞になる単語が先頭にあると関数っぽく見えますね・・・

vector<vector<int>> kSmallestPairs(vector<int>& nums1, vector<int>& nums2, int k) {
long long int left = nums1.front() + nums2.front();
long long int right = nums1.back() + nums2.back() + 1;
long long int mid;
Copy link

Choose a reason for hiding this comment

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

mid は while 文の中でしか使わないため、 while 文の中で定義したほうが良いと思います。一般に変数のスコープは、短いほうが、読んだときに認知負荷が低くなり、読みやすくなると思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

たしかにここにmidがあるのはおかしいですね

long long int left = nums1.front() + nums2.front();
long long int right = nums1.back() + nums2.back() + 1;
long long int mid;
while (left + 1 < right) {
Copy link

Choose a reason for hiding this comment

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

この二分探索は、 2 つの整数の和が i 以下となるようなペアの数が k 以上となるような最小の i を求めていますよね。その場合、 [false, false, ... false, true, ... true] と並んだ配列のうち、一番左の true の位置を求める問題と帰着できると思います。この問題を解くにあたり、区間は半開区間で考えて良いと思います。半開区間で考えた場合、 count < k の場合は、 mid の位置は false となるので、区間を狭めたときに mid の位置は区間に含めないようにしたいです。そのため、 left = mid + 1 となると思います。また、最終的に left の位置に一番左の true がきますので、以降は left を参照するのが正しいと思います。ご確認いただいてもよろしいでしょうか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、なるほど、最終的にleftとrightが一致するように二分探索を組むということですね。

priority_queue<SumPair> sum_pairs;
for (int num1 : nums1) {
for (int num2 : nums2) {
if (num1 + num2 > right) {
Copy link

Choose a reason for hiding this comment

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

right が何を表しているか分かりづらいため、いちど max_sum 等、別の変数名を割り当てたほうがよいと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

たしかにrightのまま使うと意味不明ですね

class Solution {
public:
vector<vector<int>> kSmallestPairs(vector<int>& nums1, vector<int>& nums2, int k) {
priority_queue<SumIndex, vector<SumIndex>, greater<SumIndex>> sum_index;
Copy link

Choose a reason for hiding this comment

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

greater を使わず、 operator < を定義したほうがシンプルだと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

デフォルトで最小値を取り出すpriority_queueに対して、演算子の定義で最大を取り出すようにすると逆に分かりづらくなると思い、このようにしていました。

なにをもって大小関係とするかは定義によるので、シンプルにできる方を選択するという理解をしました。

private:
struct SumIndex {
int sum;
vector<int> index;
Copy link

Choose a reason for hiding this comment

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

vector は値を格納するにあたりヒープメモリを確保するため、定数倍重くなります。避けられるのであれば避け、 step3_2 のように、固定長配列や、変数複数個を使ったほうがよいと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど、ありがとうございます。
vectorの利用には気をつけます。

class Solution {
public:
vector<vector<int>> kSmallestPairs(vector<int>& nums1, vector<int>& nums2, int k) {
priority_queue<vector<int>, vector<vector<int>>, greater<vector<int>>> sum_index;
Copy link

Choose a reason for hiding this comment

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

要素を vector にすると、何番目の要素が何を表しているのか把握しにくく、読みにくく感じます。構造体を定義し、 operator < を定義してあげたほうが、読んでいてわかりやすいと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、たしかにそのとおりですね。

if (num1 + num2 > right) {
break;
}
sum_pairs.push({num1 + num2, {num1, num2}});

Choose a reason for hiding this comment

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

emplace

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます、pushよりもこちらのほうが効率が良さそうですね
https://ja.cppreference.com/w/cpp/container/priority_queue/emplace


struct SumPair {
int sum;
vector<int> pair;

Choose a reason for hiding this comment

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

そのままtuple<int,int,int>使ってもいいのかなと思いました

Copy link
Owner Author

Choose a reason for hiding this comment

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

tupleは何が何を意味しているのかが分かりづらくなるので使いませんでした。

set<vector<int>> visited;
vector<vector<int>> answer;
sum_index.push({0, 0, 0});
while (answer.size() < k && sum_index.size() > 0) {

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.

ありがとうございます、統一しておくと読みやすくなりそうですね。

int j = sum_index.top()[2];
sum_index.pop();
answer.push_back({nums1[i], nums2[j]});
if (i + 1 < nums1.size() && !visited.contains({i + 1, j})) {

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.

ここは外に出すと逆に複雑になってしまいそうなので、共通化などせずにこのようにしています。

sum_index.push({nums1[i + 1] + nums2[j], i + 1, j});
visited.insert({i + 1, j});
}
if (j + 1 < nums2.size() && !visited.contains({i, j + 1})) {

Choose a reason for hiding this comment

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

ちなみにvisitedを使わない方法もありますのでご参考までに
ryoooooory/LeetCode#17

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 +8 to +26
priority_queue<SumIndex, vector<SumIndex>, greater<SumIndex>> sum_index;
vector<vector<int>> minimum_k_pair;
set<vector<int>> visited;
sum_index.push({nums1[0] + nums2[0], 0, 0});
while (minimum_k_pair.size() < k && sum_index.size() > 0) {
int i = sum_index.top().num1_index;
int j = sum_index.top().num2_index;
sum_index.pop();
minimum_k_pair.push_back({nums1[i], nums2[j]});
if (i + 1 < nums1.size() && !visited.contains({i + 1, j}) && (j == 0 || visited.contains({i + 1, j - 1}))) {
sum_index.push({nums1[i + 1] + nums2[j], i + 1, j});
visited.insert({i + 1, j});
}
if (j + 1 < nums2.size() && !visited.contains({i, j + 1}) && (i == 0 || visited.contains({i - 1, j + 1}))) {
sum_index.push({nums1[i] + nums2[j + 1], i, j + 1});
visited.insert({i, j + 1});
}
}
return minimum_k_pair;

Choose a reason for hiding this comment

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

結構行数が多いので、ある程度の粒度で関数に切り出したり、空白行が入っているともっと見通しが良くなるなと思いました。

Copy link

@seal-azarashi seal-azarashi Sep 19, 2024

Choose a reason for hiding this comment

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

手前味噌ですが、先に片方の配列の先頭要素と、もう片方の配列の要素全ての pair を priority queue をに入れてから処理を続ける方法もありますので参考までに: seal-azarashi/leetcode#10 (comment)

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 +13 to +14
int i = sum_index.top().num1_index;
int j = sum_index.top().num2_index;

Choose a reason for hiding this comment

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

SumIndex では num~_index と命名されているものが i, j となるのに少し違和感を感じました。while 文の中でだけ、頭の中で変数名マップを張っていないと正確に実装が理解出来ないので、読みづらくなりそうだなという印象です。
とはいえ今の実装でそのまま num~_index に置き換えると文字数が大変多くなってしまうのは悩ましいところです...

Copy link
Owner Author

Choose a reason for hiding this comment

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

numX_indexにしてしまうと一行が長くなってしまうのでこのようにしていました。
i,jが分かりづらいというのはごもっとですね。
#25 (comment)
と合わせて、関数抽出などで何か改善できそうな気がします。

priority_queue<SumIndex, vector<SumIndex>, greater<SumIndex>> sum_index;
set<vector<int>> visited;
vector<vector<int>> minimum_k_pairs;
sum_index.push({nums1[0] + nums2[0], 0, 0});
Copy link

Choose a reason for hiding this comment

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

個人的には、sumとindexが入っていることはここを見る(もしくはstructの中身を見る)かすればすぐわかるので、やや冗長に感じました。
どちらかというと、これがK番目までに入る「候補」である(実際にK番目までに入ると決まったわけではない)ということを知りたいので、candidatesなどの方がいいかなと思いました。

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど、ありがとうございます。
「候補」のニュアンスをもたせるほうがコードの中身がわかりやすくなりそうですね。

}
};

int count_less_pairs(vector<int> nums1, vector<int>nums2, int num, int limit) {
Copy link

Choose a reason for hiding this comment

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

ここ、vector が毎回コピーされますね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

これは良くないですね・・・
ありがとうございます

int sum;
vector<int> index;

bool operator> (const SumIndex& other) const {
Copy link

Choose a reason for hiding this comment

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

https://brevzin.github.io/c++/2019/07/28/comparisons-cpp20/
比較演算子を定義するのは色々と大変な話があるので話半分に目を通しておくといいでしょう。話半分にです。

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 +13 to +14
int i = sum_index.top().num1_index;
int j = sum_index.top().num2_index;
Copy link

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/language/structured_binding
C++17 以降で良ければ、structured binding が使えます。

Copy link
Owner Author

Choose a reason for hiding this comment

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

なるほど、ありがとうございます。

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