Median of push.back vector

[hrxs1]

Hello everyone,

I am currently teaching C++ to myself using the book of B. Stroustrop.

In one task, one shall write program that pushes back input values into a vector and then gives out the median.
It's possible to compile it without any problems.

However, I'm always getting following error message and the program is aborting when executing it:

http://fs5.directupload.net/images/160622/oj9kudzv.png
(expression: vector subscript out of range)

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
#include <math.h>
#include <fstream>
#include "square.h"

using namespace std;

// compute mean and median temperatures
int main()
{
	vector<double> temps; // temperatures
	for (double temp; cin >> temp; ){ // read into temp
		temps.push_back(temp); // put temp into vector
// compute mean temperature:
	double sum = 0;
	for (int x : temps) sum += x;
	cout << "Average temperature: " << sum / temps.size() << '\n';
	//compute median temperature:
	sort(temps.begin(), temps.end()); // sort temperatures
	cout << sizeof(temps);
	cout << "Median temperature: " 
<< (temps[temps.size()/2 - 1] - temps[temps.size()/2] ) /2 << '\n';}
}

When I leave out the "- 1", I don't get any problems. It works with -1 only, but without /2 in the first term as well.
I also tried using even and uneven sizes of the vector, but I am always getting this error. The sizeof(temps); line gives out "wrong" sizes of the vector too. If I input for instance "3 4 5" when running the program, it shows "16" for sizeof(temps); and then aborts, showing the abovementioned message.

So, I think there is something wrong in my code.

I am using VisualStudio2015 on Win7 x86.

Thanks in advance,
hrxs1

[Thomas1965]

The problem is with your nested loop.
When the inner loop is executed there is only one element in the vector.
This leads to an index of -1 at the end, which leads to the error msg.

[Chervil]

Several issues.
Line 19: for (int x : temps) sum += x; x should be double, not int.

Line 23, cout << sizeof(temps); this value is of no interest. Your program needs to know only the value of temps.size()

Line 25: << (temps[temps.size()/2 - 1] - temps[temps.size()/2] ) /2 << '\n';}
More than one problem.
First, the two elements to be averaged should be added together, instead of subtracting one from the other.

Second, you need to arrange for the subscripts to be either
adjacent elements, when temps.size() is even.
the same element, when temps.size() is odd.

[hrxs1]

Thank you Thomas1965 and Chervil for your replies!
I thought about your ideas and tried to write them in my code. Now everything seems to work well, is the code okay?
And is there an article or something that explains, why it is stupid to put the code for median calculation directly inside the loop?

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
#include <math.h>
#include <fstream>
#include "square.h"

using namespace std;

double median(vector<double> medi) {
	int size = medi.size();
	double tmedian;
	if (size % 2 == 0) { // even
		tmedian = (medi[medi.size() / 2 - 1] + medi[medi.size() / 2]) / 2;
		cout << "Median temperature: " << tmedian << '\n';
	}

	else //odd
		tmedian = medi[medi.size() / 2];
	cout << "Median temperature: " << tmedian << '\n';
	return (tmedian);
}


// compute mean and median temperatures
int main()
{
	vector<double> temps; // temperatures
	for (double temp; cin >> temp; ) { // read into temp
		temps.push_back(temp); // put temp into vector
							   // compute mean temperature:
		double sum = 0;
		for (double x : temps) sum += x;
		cout << "Average temperature: " << sum / temps.size() << '\n';
		// compute median temperature:
		sort(temps.begin(), temps.end()); // sort temperatures
		cout << "size of temps: " << temps.size() << "\n";
		median(temps);

	}
}

[Chervil]

is the code okay?

The results seem valid.

However in terms of design/style it isn't what I'd recommend.

And is there an article or something that explains, why it is stupid to put the code for median calculation directly inside the loop?

I don't think I'd use the word 'stupid'. Its really a matter of meeting the requirements which were asked for (in the question, or by the customer who is paying for you to write a program for them). If you are asked to show the median and average after each value has been entered, then your program is somewhat ok. Even in that case it might be improved. But if you need only the average and median of all the values then I'd take the calculations outside the loop, and do them at the end.

I'd also remove the cout statements from the function median(). Instead you might do something like this:

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

using namespace std;

double median(vector<double> medi) 
{
    sort(medi.begin(), medi.end());     // sort temperatures
            
    double tmedian;
    if (medi.size() % 2 == 0)           // even
        tmedian = (medi[medi.size() / 2 - 1] + medi[medi.size() / 2]) / 2;
    else                                // odd
        tmedian = medi[medi.size() / 2];
    
    return tmedian;
}

double average(const vector<double> & temps) 
{
    double sum = 0;
    for (double x : temps) 
        sum += x;
        
    return sum / temps.size();
}


int main()
{
    vector<double> temperatures;
            
    for (double temp; cin >> temp; ) 
    { 
        temperatures.push_back(temp); 
    }
        
    // compute mean and median temperatures
    
    cout << "Average temperature: " << average(temperatures) << '\n';
    cout << "Median temperature:  " << median(temperatures)  << '\n';
    cout << "size of temps:       " << temperatures.size()   << '\n';

}

The code I posted here might be improved, I chose to pass the temperature vector to the median function by value, it could be passed by reference instead - depending on whether or not the original array should be sorted or left in the original order.

[hrxs1]

Much thanks.
You helped me a lot.