why is sorting not working in vector of pairs

Question

I am trying to sort a vector of pairs but I am not able to understand what is error in this code.

#include <bits/stdc++.h>

bool sortbysec(const pair<int,int> &a, const pair<int,int> &b) { 
    return (a.second < b.second); 
} 

using namespace std;

int main() {
    // ios_base::sync_with_stdio(false);
    // cin.tie(NULL);

    long int t, i;
    cin >> t;
    vector<pair<int, int>> vect;
    pair<int, int> tmp;
    for (i = 0; i < t; i++) {
        cin >> tmp.first;
        vect.push_back(tmp);
    }
    sort(vect.begin(), vect.end(), sortbysec); 

    return 0;
}

Someone please help me to understand what is wrong in this piece of code.

[Why should I not #include ?](https://stackoverflow.com/q/31816095/5910058) — Jesper Juhl, Jun 06 '20 at 07:59
What error are you seeing? You aren't printing the sorted list so how do you know it's not correct? Please show a [mre] — Alan Birtles, Jun 06 '20 at 08:00
[Why is "using namespace std;" considered bad practice?](https://stackoverflow.com/q/1452721/5910058) — Jesper Juhl, Jun 06 '20 at 08:00
Presumably `const pair &a` is not compiling due to lack of `std::` on `pair` — Alan Birtles, Jun 06 '20 at 08:04
Your custom comparator is comparing only the `.second` element of each pair, which you never set to anything. Value initialization will set that member of `tmp` to to `0`, meaning all of your elements are the same as far as your comparator is concerned. So no wonder it doesn't do what you want; rather it does *exactly* what you told it do to. — WhozCraig, Jun 06 '20 at 08:16

Anil Gupta · Answer 1 · 2020-06-06T09:12:39.453

Firstly, i would like to point out your errors
1. Always write using namespace std after header files, since you are writing below your custom comparator so the compiler could not able to resolve what pair is, since it belongs to std namespace.
2. Since you are sorting your pair by second value you should take second value in input as well.

#include <bits/stdc++.h>
using namespace std;

bool sortbysec(const pair<int,int> &a, const pair<int,int> &b) 
{ 
    return (a.second < b.second); 
} 

int main(){
    // ios_base::sync_with_stdio(false);
    // cin.tie(NULL);

    long int t, i;
    cin >> t;
    vector<pair<int, int> > vect;
    pair<int, int> tmp;

    for(i=0; i<t; i++){
        cin>>tmp.first >> tmp.second;
        vect.push_back(tmp);
    }
    cout << "Before Sorting" << "\n";
    for (int i = 0; i < t; i ++) {
        cout <<vect[i].first << " " << vect[i].second << "\n";
    }
    sort(vect.begin(), vect.end(), sortbysec);
    cout << "after Sorting" << "\n";
    for (int i = 0; i < t; i ++) {
        cout <<vect[i].first << " " << vect[i].second << "\n";
    }

    return 0;
}

I have refactored your code, hope this helped.
Input
4
2 3
4 1
2 6
3 2
Output
Before Sorting
2 3
4 1
2 6
3 2
after Sorting
4 1
3 2
2 3
2 6

Thanks a lot.. The mistake that I was making was writing 'using namespace std' after defining function as you mentioned. — Divya garg, Jun 06 '20 at 15:11

Coral Kashri · Answer 2 · 2020-06-06T09:15:19.990

You are sorting only by the pair .second value, and input only the .first value. To solve this issue you can:

1. Change your sort function to sort by pair .first value:

bool sortbyfirst(const pair<int,int> &a, const pair<int,int> &b) { 
    return (a.first < b.first); 
}

2. Input your to pair .second value:

for (i = 0; i < t; i++) {
    cin >> tmp.first;
    cin >> tmp.second; // Add this line
    vect.push_back(tmp);
}

3. {{Combine}} Input both .first & .second, and sort by both:

bool sort_by_first_and_sec(const pair<int,int> &a, const pair<int,int> &b) {
    return a.first <= b.second && a.second < b.second; 
}

Deepak Tatyaji Ahire · Answer 3 · 2020-06-06T09:16:09.127

I agree with @WhozCraig. Your custom comparator is comparing only the .second element of each pair, which you never set to anything.

Try taking input for the .second value.

Have a look at the following implementation:

#include <iostream> 
#include <utility>
#include <vector> 
#include <algorithm> 

bool sortbysec(const std::pair<int,int> &a, const std::pair<int,int> &b) { 
    return (a.second < b.second); 
} 

int main() {

    int t, i;
    std::cin >> t;

    std::vector<std::pair<int, int>> vect;

    std::pair<int, int> tmp;

    for (i = 0; i < t; i++) {
        std::cin >> tmp.first;
        std::cin >> tmp.second;
        vect.push_back(tmp);
    }

    sort(vect.begin(), vect.end(), sortbysec); 

    for (i = 0; i < t; i++) {
        std::cout<<vect[i].first<<" "<<vect[i].second<<std::endl;
    }

    return 0;
}

Input:

Output:

PS: Have a look on Why should I not #include bits/stdc++.h?

Also, Why is “using namespace std;” considered bad practice?

score 1 · Answer 4 · answered Jun 06 '20 at 09:57

More modern (and compact) version of your code (C++14):

#include <algorithm>
#include <iostream>
#include <utility>
#include <vector>

int main() {
    long int t;
    std::cin >> t;
    std::vector<std::pair<int, int>> v(t);
    for (auto &i : v)
        std::cin >> i.first >> i.second;
    std::sort(v.begin(), v.end(), [](const auto &a, const auto &b) {
        return a.second < b.second;
    });
    for (auto &i : v)
        std::cout << i.first << ' ' << i.second << '\n';
    return 0;
}

why is sorting not working in vector of pairs

4 Answers4