qsot array of Time objects - help

I have been trying to figure this out for the past few hours so it's about time i search for help.

I was to use the Time class library and sort an array of time objects using the qsort function. I beleive my compare function is the problem and have tried different variations but still can't seem to grasp it.

Code:

---


#include <iostream>
#include "ccc_time.h"


using namespace std;
int tCompare(const void* , const void*) ;

void main()
{
   

    Time* times[7] = {new Time(10, 13, 55), new Time(15, 13, 59), new Time(18, 7, 20), new Time(16, 10, 39),
                      new Time(9, 13, 45), new Time (16, 15, 24), new Time(17, 5, 8)};
   

   

    cout << "unsorted times: " << endl;

    for (int i =0; i < 7; i++)
        cout<< times[i]->get_hours() << ":" << times[i]->get_minutes() << ":" << times[i]->get_seconds() << endl;
   
    /* can't get to sort, beleive its something wrong with my compare function */
    qsort(times, 7, sizeof(Time), tCompare);
   
    cout << endl;
    cout << "sorted times: " << endl;

    for (int i =0; i < 7; i++)
        cout<< times[i]->get_hours() << ":" << times[i]->get_minutes() << ":" << times[i]->get_seconds() << endl;
   
    system ("pause");
}

int tCompare(const void* ptr1, const void* ptr2)

{
    Time time1, time2;

    time1 = *(Time *)ptr1; 
    time2 = *(Time *)ptr2; 
   
    /* can't get to sort, beleive its something wrong with my compare function,
    this is not frist variation iv'e tried of the compare*/
   
    if(time2.get_hours() - time1.get_hours() > 0)
        return -1;
    else if (time2.get_hours() - time1.get_hours() == 0 && time2.get_minutes() - time1.get_minutes() > 0)
        return -1;
    else if (time2.get_hours() - time1.get_hours() == 0 && time2.get_minutes() - time1.get_minutes() == 0
            && time2.get_seconds() - time1.get_seconds() > 0)
        return -1;
    else if (time2.get_hours() - time1.get_hours() == 0 && time2.get_minutes() - time1.get_minutes() == 0
            && time2.get_seconds() - time1.get_seconds() == 0)
        return 0;
    else
        return 1;
}

---

I've got it to work. I made my array a normal array instead of an array of pointers. I don't understand how that fixed it but it works.

my first question is why are you accepting void* parameters in the function, and then casting, using the incorrect, c-style cast, to Time*? that makes absolutely no sense in a strongly-typed language like C++.

Your first incarnation didn't work because of these lines:

Code:

---

    Time time1, time2;
    time1 = *(Time *)ptr1;
    time2 = *(Time *)ptr2;

---

It should be

Code:

---

    Time *time1, *time2;
    time1 = (Time *)ptr1;
    time2 = (Time *)ptr2;

---

and you access them like

Code:

---

time2->get_hours() - time1->get_hours()
etc.

---

You could rewrite it something like this:

Code:

---

int tCompare(const void* ptr1, const void* ptr2)
{
    Time *time1 = (Time *)ptr1;
    Time *time2 = (Time *)ptr2;

    // change int to whatever type hours, minutes, and seconds are
    int dh = time2->get_hours()  - time1->get_hours();
    int dm = time2->get_minutes() - time1->get_minutes();
    int ds = time2->get_seconds() - time1->get_seconds();

    if (dh > 0 || dh == 0 && dm > 0 || dh == 0 && dm == 0 && ds > 0)
        return -1;
    return !(dh == 0 && dm == 0 && ds == 0);
}

---

Quote:

---

Originally Posted by oogabooga

Your first incarnation didn't work because of these lines:

Code:

---

    Time time1, time2;
    time1 = *(Time *)ptr1;
    time2 = *(Time *)ptr2;

---

It should be

Code:

---

    Time *time1, *time2;
    time1 = (Time *)ptr1;
    time2 = (Time *)ptr2;

---

and you access them like

Code:

---

time2->get_hours() - time1->get_hours()
etc.

---

---

Thank you! This would explain why -> operators where not working in one of my versions that I tried. Also you illustrated what was wrong simply and clearly in a time efficient manner. Thank you very much.

it would be better to define the function as follows:

Code:

---

int tCompare(const Time* time1, const Time* time2)
{
    // change int to whatever type hours, minutes, and seconds are
    int dh = time2->get_hours()  - time1->get_hours();
    int dm = time2->get_minutes() - time1->get_minutes();
    int ds = time2->get_seconds() - time1->get_seconds();
 
    if (dh > 0 || dh == 0 && dm > 0 || dh == 0 && dm == 0 && ds > 0)
        return -1;
    return !(dh == 0 && dm == 0 && ds == 0);
}

---

Thank you, cleaning up my code and making it more efficient is something I strongly need to work on.

It would be better to get rid of the pointers altogether. They are leaking memory because you aren't deleting.
I still don't understand why no one has yet complained about void main. Change it to its proper int main, post haste.
Last, but not least, a different way of doing this is to use operator overloading. You should try it just to get the hang of both ways.
(Does not work with pointers, mind you!)

Also have a look at:
SourceForge.net: Do not remove parameter names - cpwiki
SourceForge.net: Pause console - cpwiki