Sorting a vector of objects containing two data members using std::sort

Question

I'm trying to store data from a file into a vector of objects and then sorting the data members but I'm getting the errors "Cannot determine which instance of overloaded function "sort" is intended". I've tried using lambdas with sort and also thought it might be the way I've created my comparison function (is it a.hour > b.hour or should I use a.getHour() and b.getHour()?) I actually want to sort the vector by both hours and minutes but testing it on only hours first doesn't seem to work. This is what I have so far

#include <iostream>
#include <vector>
#include <string>
#include <algorithm>
#include <fstream>
using namespace std;

class time {
    int hour;
    int minute;
public:
    time(int h, int m) {
        hour = h;
        minute = m;
    }
    int getHour() { return hour; }
    int getMinute() { return minute; }
};

class Times {
    vector<time> t;
public:
    Times(string fileName) {
        //
    }

    void parse(ifstream& file) {
        //
        sort.(t.begin(), t.end(), lowerThan);
        //sort.(t.begin(), t.end(), [] (time& a, time& b) { return a.hour < b.hour; })
    }

    void display() {
        for (size_t i = 0; i < t.size(); i++) {
            cout << t[i].getHour() << ":" << t[i].getMinute() << endl;
        }
    }

    static bool lowerThan(time& a, time& b) { return a.getHour() < b.getHour(); }
};

int main() {
    Times times("File.txt");
    times.display();

    system("pause");
    return 0;
}

First [you should not use `using namespace std;`](http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice). Second it looks like you have a typo. You have `sort.(...` not `sort(...` — NathanOliver, Mar 15 '16 at 17:50
Avoid `using namespace std`, Even more when you use name used in `std` as `time`. — Jarod42, Mar 15 '16 at 17:52
Hi I've seen this in several places, but why to avoid using namespace std; ? — Khalil Khalaf, Mar 15 '16 at 17:56
BTW, even without `using namespace std;`, it seems that your class `time` conflicts anyway with (C) function `time`, so use your own namespace, or rename your class `time`. — Jarod42, Mar 15 '16 at 18:05

score 2 · Accepted Answer · edited May 23 '17 at 12:23

There are several issues in your code.

You should not use using namespace std; to avoid name clashing.

Moreover, unfortunately your time (lowercase) class conflicts with another time standard identifier. Just use Uppercase convention for naming classes.

In addition, you may want to mark your Time::getHour() and Time::getMinute() methods as const, since they don't modify the internal state of Time objects.

You also have a typo with sort calls, since you have a dot following sort.

And, in C++11/14, I'd suggest you using range-based for loops instead of explicit for with integer indexes.

I've refactored your code a little bit considering those aspects, and it now works, with both the lowerThan() static method and the lambda. Feel free to study it.

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

class Time {
    int hour;
    int minute;
public:
    Time(int h, int m) : hour(h), minute(m) {
    }

    int getHour() const { return hour; }
    int getMinute() const { return minute; }
};

class Times {
    std::vector<Time> t;

    static bool lowerThan(const Time& a, const Time& b) { 
        return a.getHour() < b.getHour(); 
    }

public:
    Times() {
        // Test data
        t.push_back(Time{10, 10});
        t.push_back(Time{9, 20});
        t.push_back(Time{8, 30});

        //std::sort(t.begin(), t.end(), lowerThan);

        std::sort(t.begin(), t.end(), [] (const Time& a, const Time& b) { 
            return a.getHour() < b.getHour(); 
        });
    }

    void display() {
        for (const auto& x : t) {
            std::cout << x.getHour() << ":" << x.getMinute() << '\n';
        }
    }
};

int main() {
    Times times;
    times.display();
}

Note also that if you define a custom operator< overload for sorting instances of the Time class, you can simply call std::sort() without any custom comparator, and your custom implementation of operator< is automatically picked up by the compiler:

class Time {
...
    friend bool operator<(const Time& a, const Time& b) {
        return a.getHour() < b.getHour();
        // ... or a more complete comparison, including minutes.
    }   
};

EDIT

As suggested in a comment by @DieterLücking, you can use std::tie() for the operator< implementation (live here on Ideone):

#include <tuple>  // for std::tie

class Time {
    ...
public:

friend bool operator<(const Time& a, const Time& b) {
    return std::tie(a.hour, a.minute) < std::tie(b.hour, b.minute);
}   

};

For sorting: `std::tie(a.hour, a.minute) < std::tie(b.hour, b.minute);` or `std::make_tuple(a.getHour(), a.getMinute()) < std::make_tuple(b.getHour(), b.getMinute());` if the operator is no friend — , Mar 15 '16 at 18:38

Sorting a vector of objects containing two data members using std::sort

1 Answers1