macOS What is wrong with this C++ program?

[pantera]

Hi,

I still can't figure out where this program goes wrong. It keeps producing undefined behavior... Please help. Thank you.

// Exercise 8.12 - Version 1

#include "std_lib_facilities.h"

/*
	[COLOR="Red"]Write a function that finds the smallest & the largest element of a vector
	argument and also compute the mean and the median. Do not use global variables.
	Either return a struct containing results or pass them back through reference
	arguments. Which of the 2 ways of returning several result values do you prefer and why?[/COLOR]

*/

struct Stats_results {
	double mean;
	double median;
	double max;
	double min;
};

Stats_results compute_stats(vector<double> data)
{
	Stats_results results;
	
	// mean
	double sum = 0;
	for (unsigned int i = 0; i < data.size(); ++i)
		sum += data[i];
	results.mean = sum / data.size();
	
	// median
	sort(data.begin(), data.end());
	if (data.size()%2==0) 
		results.median = (data[data.size()/2] + data[data.size()/2 - 1])/2;
	else
		results.median = data[data.size()/2];
		
	// max
	results.max = data[data.size() - 1];
	
	// min
	results.min = data[0];
	
	return results;
}

int main()
try {
	Stats_results myresults;
	cout << "Enter some numbers for a vector: \n";
	vector<double> v;
	double x;
	while (cin >> x)
		v.push_back(x);
	
	if (v.size() == 0) error("Empty vector!\n"); // make sure the user input data	
	
	compute_stats(v);
	cout << "Mean: " << myresults.mean << '\n';
	cout << "Median: " << myresults.median << '\n';
	cout << "Max: " << myresults.max << '\n';
	cout << "Min: " << myresults.min << '\n';
}

catch (runtime_error e) {
	cout << e.what() << '\n';
}

 

[lee1210]

Where do you think myresults is being set to the return value of compute_stats?

I think you just found a reason not to use this method, and push you toward passing some reference arguments.

-Lee

P.S. You said you're teaching yourself, so this isn't like traditional HW, but you should mention if you don't want people to just solve your problem and post the code. You will learn less if they do.

 

[chown33]

I'm no C++ expert, but it looks like myresults in main() is never assigned a value. Specifically, not the return value from compute_stats(). Therefore, it's an uninitialized local variable whose value is being used incorrectly.

 

[pantera]

Where do you think myresults is being set to the return value of compute_stats?

I think you just found a reason not to use this method, and push you toward passing some reference arguments.

-Lee

P.S. You said you're teaching yourself, so this isn't like traditional HW, but you should mention if you don't want people to just solve your problem and post the code. You will learn less if they do.

Thank you for the hint. This is an exercise from the book "Programming: Principles and Practice using C++". I have done this exercise for one version and it produces the correct results. Then, I tried to modify it a bit using different functions (I'm trying to come up with different ways to get to the solution). I will try to look at it again & see how I can fix it. Thank you for your time.

My previous version is like below:

// Exercise 8.12 - Version 4


#include "std_lib_facilities.h"

[COLOR="Red"]/*
	Write a function that finds the smallest & the largest element of a vector
	argument and also compute the mean and the median. Do not use global variables.
	Either return a struct containing results or pass them back through reference
	arguments. Which of the 2 ways of returning several result values do you prefer and why?

*/[/COLOR]

struct stats {
	vector<double> v;
	double mean;
	double median;
	double max;
	double min;
};

void fstats(stats& results)
{
	// mean
	double sum = 0;
	for (unsigned int i = 0; i < results.v.size(); ++i)
		sum += results.v[i];
	results.mean = sum / results.v.size();	
	
	// median
	sort(results.v.begin(), results.v.end());
	if (results.v.size()%2 == 0) 
		results.median = (results.v[results.v.size()/2] + results.v[results.v.size()/2-1])/2;
	else
		results.median = results.v[results.v.size()/2];
	
	// max & min
	results.min = results.v[0];
	results.max = results.v[results.v.size()-1];
		
}

int main()
try {
	stats myresults;
	cout << "Enter some elements: \n";
	double x;
	while (cin >> x)
		myresults.v.push_back(x);
	fstats(myresults);
	cout << "Mean: " << myresults.mean << '\n';
	cout << "Median: " << myresults.median << '\n';
	cout << "Max: " << myresults.max << '\n';
	cout << "Min: " << myresults.min << '\n';
}

catch (runtime_error e) {
	cout << e.what();
}

 

[pantera]

Where do you think myresults is being set to the return value of compute_stats?

I think you just found a reason not to use this method, and push you toward passing some reference arguments.

-Lee

P.S. You said you're teaching yourself, so this isn't like traditional HW, but you should mention if you don't want people to just solve your problem and post the code. You will learn less if they do.

I've been finding learning C++ really hard. So I need a lot of help from online forums. I don't have the opportunity to attend a class, nor do I have a friend who is a CS that I can turn to for help when need arises. I'm learning it in my free time (whenever I have free time, even during X-mas/New Year time...). Reading the chapter and understanding the idea behind many difficult concepts takes time, and this is not enough. Doing exercises at the end of chapters and reading other people's codes should be essential, practicing writing codes, etc... All these things in reality takes so much time, so I've been going much slower than a normal uni C++ intro course (they've covered the whole book of 20 chapters in 14 weeks). I've spent 14 weeks already, but still at the Chapter 10/11)... Thank you for taking time to reply and giving the hint...

 

[pantera]

I'm no C++ expert, but it looks like myresults in main() is never assigned a value. Specifically, not the return value from compute_stats(). Therefore, it's an uninitialized local variable whose value is being used incorrectly.

Thank you, chown33, for the hint...

 

[Sander]

Your previous version worked because you passed the "stats" structure to your function by reference, with the function "filling it in" for you.

When you modify the code to make the function return the stats, you must also make sure to change the calling site and assign this function result to some local variable for further use.

One small idiomatic thing: You pass the vector "by value" to your function, which means a copy is made. For small data sets this isn't a problem, but for a vector containing a gazillion entries, it is. That's why experienced C++ programmers usually pass these by const reference.

Now, in your case, you actually modify the vector (because you're sorting it), and modifying the caller's data inside the callee is usually considered "impolite" (if it was a const reference, it wouldn't even compile). Since you'd have to copy the data anyway with your implementation, you might as well do so during the call, so your code is fine. However, since passing a vector by value is the exception, you might want to add a line of comment explaining why you're doing it.