getting wrong output while sorting in C++

Question

The following C++ code sorts an array in descending order using qsort:

#include<iostream>
#include<cstdio>
#include <stdlib.h>
using namespace std;

struct player {
  double data;
  int index;
};

struct player A[] = {{0.690277,0}, {0.517857,1}, {0.780762,2}, {0.0416667,3}, {0.0416667,4}};

int compare (const void * a, const void * b)
{
   return ( ((struct player*)b)->data - ((struct player*)a)->data );
}

int main ()
{
  int n;
  qsort (A, 5, sizeof(struct player), compare);
  for (n=0; n<5; n++)
  printf ("data=%lf, index=%d\n", A[n].data, A[n].index);
  return 0;
}

But I am getting output like this:

data=0.517857, index=1
data=0.780762, index=2
data=0.041667, index=3
data=0.041667, index=4
data=0.690277, index=0

Is there anything wrong in the code?

Not that there's anything wrong with qsort, but is there a reason you chose it? Your code feels very C-like in general. There's a few things in there I'd update to be more C++ like, as this question is tagged as such. — AndyG
– AndyG, Commented Sep 28, 2016 at 15:43

weltensturm · Accepted Answer · 2016-09-28 15:41:19Z

8

In compare, you are subtracting two sub-1 doubles and casting them to an int, the result will in most cases be 0. Instead of subtracting you should compare them and return -1/1.

answered Sep 28, 2016 at 15:41

weltensturm

2,18217 silver badges17 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

user5411115 Over a year ago

can you please show me what I would write inside the compare function? @weltensturm

weltensturm Over a year ago

Take a look at utnapistim's answer.

utnapistim · Accepted Answer · 2016-09-28 15:53:25Z

Consider using this compare instead:

int compare (const void * a, const void * b)
{
    auto x = reinterpret_cast<const player*>(a);
    auto y = reinterpret_cast<const player*>(b);
    if(x->data < y->data)
        return -1;
    if(x->data > y->data)
        return 1;
    return 0;
}

That said, this style of coding is ancient/deprecated/bad practice.

Consider writing similar to this instead:

#include<iostream>
#include <algorithm>

struct player {
  double data;
  int index;

    bool operator < (const player& p) const
    {
        return data < p.data;
    }
};

auto A = std::vector<player>{
    {0.690277,0}, {0.517857,1},
    {0.780762,2}, {0.0416667,3}, {0.0416667,4}
};

int main ()
{
    std::sort(std::begin(A), std::end(A));
    for(const auto& x: A)
        std::cout << "data=" << x.data << ", "
            << "index=" << x.index << "\n";
}

Suggested changes:

don't import std names globally
don't mix cstdio and iostreams (only include one of them)
use std::vector or std::array instead of native array
define the sorting order in the interface of the class (bool operator <). (this should also imply that you define the other arithmetic operators - it is good practice and avoids subtle bugs later, but it is not required for this particular implementation to compile and work)
use std::sort instead of qsort
don't use raw pointers (using them like this is a source for bugs)

Collectives™ on Stack Overflow

getting wrong output while sorting in C++

2 Answers 2

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related