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;. Second it looks like you have a typo. You have sort.(... not sort(... — NathanOliver
– NathanOliver, Commented Mar 15, 2016 at 17:50
Avoid using namespace std, Even more when you use name used in std as time. — Jarod42
– Jarod42, Commented Mar 15, 2016 at 17:52
Hi I've seen this in several places, but why to avoid using namespace std; ? — Khalil Khalaf
– Khalil Khalaf, Commented Mar 15, 2016 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
– Jarod42, Commented Mar 15, 2016 at 18:05

Community · Accepted Answer · 2017-05-23 12:23:47Z

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

Collectives™ on Stack Overflow

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

1 Answer 1

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related