Oddities in a C++ sorting function

Question

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

// To Do: Finish this function
void yourFunction(int N, vector<float>&vec, float &m, float &n){
sort(vec.begin(), vec.end());
n=vec[0];
m=vec[N];
}

int main()
{
    int N;
    float m,n;
    cout << "Please enter the length of array." << endl;
    cin >> N;
    float *p = (float*) malloc(sizeof(float)*N);
    cout << "Please enter the numbers in your array.";
    for(int i=0;i<N;i++)
        cin >> *(p+i);
    vector<float>vec(p[0],p[N-1]);
    yourFunction(N,vec,m,n);
    cout << "The largest number in your array is " << m << endl;
    cout << "The smallest number in your array is " << n << endl;
    return 0;
}

So this is a C++ program that is meant to identify the largest and smallest number in a user-inputted array. Code::Blocks 16, C++0X standards. In its current state, however, when I input 1 2 3 4 5 for the array, the program's output is as following:

https://i.stack.imgur.com/phktl.png

What is the problem here? I am an amateur coder and am probably making some dumb mistakes I didn't notice. :P

Why are you using `float *p = (float*) malloc(sizeof(float)*N);`? Just add the numbers to the vector directly using `push_back`. — NathanOliver, Jul 26 '17 at 13:51
Sorry, I should clarify on some parts, the first initialization to a pointer is code that is required on my assignment, I cannot edit or delete it, but I decided to use a vector instead. — Nameless King, Jul 26 '17 at 13:54
In that case there is no need for a vector. You can sort a dynamic array by passing pointers to `sort`. — NathanOliver, Jul 26 '17 at 13:55
Oh, that makes things a lot simpler, thanks! So would I pass *(p[0],p[N-1]) or something? — Nameless King, Jul 26 '17 at 13:57
`m=vec[N];` should be N-1, but use front() and back() methods. Also you do not need to pass size, as you can get it directly from vector. — Hakes, Jul 26 '17 at 13:59

NathanOliver · Accepted Answer · 2017-07-26T14:11:33.327

Your biggest issue here is that

vector<float>vec(p[0],p[N-1]);

doesn't construct the vector you think it does. p[0] and p[N-1] are floats, not pointers. So what you do is construct a vector with p[0] number of elements all with the value of p[N-1]. What you need if you want to construct a vector for an array is

vector<float>vec(p,p + N);

You also have an issue with

m=vec[N];

Since N is the size of the vector vec[N] is not a valid element. It is one past the end. What you need is

m=vec[N - 1];

Do note that the vector is not needed here at all. You can just take a float* in yourFunction and sort that directly. That would look liked

void yourFunction(int N, float* data, float &m, float &n){
    std::sort(data, data + N);
    n = data[0];
    m = data[N - 1];
}

int main()
{
    int N;
    float m,n;
    cout << "Please enter the length of array." << endl;
    cin >> N;
    float *p = (float*) malloc(sizeof(float)*N);
    cout << "Please enter the numbers in your array.";
    for(int i=0;i<N;i++)
        cin >> *(p+i);
    yourFunction(N,p,m,n);
    cout << "The largest number in your array is " << m << endl;
    cout << "The smallest number in your array is " << n << endl;
    return 0;
}

Even though you said you cannot change the code in main I would also like to let you know that new should be preferred over malloc

@NamelessKing I changed `sort` to `std::sort`. Does that fix it? [Here](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) is some good reading on why not to use `using namespace std;`. — NathanOliver, Jul 26 '17 at 14:07
Thanks a lot I should probably read up on that, just wanted to point out that in your example `vec` was not declared — Nameless King, Jul 26 '17 at 14:09
@NamelessKing Oops. Forgot to change those lines. I just fixed it. — NathanOliver, Jul 26 '17 at 14:11

Gaurav Sehgal · Answer 2 · 2017-07-26T13:58:56.950

First your vector creation is wrong.

vector<float>vec(p[0],p[N-1]);//p[0] and p[N-1] are values.You need address range

vector<float>vec(p,p+N); //begin with P(including) and end with p+N(excluding)

Then,

n=vec[0];
m=vec[N];  //Last element at N-1
}

should be

m = vec[N-1];

Also why do you have both array and vector.

Either use

std::sort(p,p+N); //and get rid of the vector

or directly put values into vector.

vector<float> vec(N);
for(int i=0;i<N;i++)
   cin>>vec[i];

Oddities in a C++ sorting function

2 Answers2