Mean/Medium Function Questions

[Aeon221]

This function is part of a much larger program that I am in the process of writing, and I haven't yet "plugged it in". The intent is for it to go through a vector<int> that contains past data, find the mean and median, and then return either mean or median depending on the value contained in the argument (which is derived from a bool and is thus only either 1 or 0).

It compiles fine, but I want to make sure that:

1) the compiler warning that not all control paths return a value won't be an issue and that

2) this does what I'm expecting it to do.

Thanks!

Oh, and if there's a better way to do any/all of this, I'm all ears. I feel like I'm reinventing the wheel, but a bit of googling didn't turn up an obvious alternative.

int Food::get_avg_gather(int optimist) {
//for readers of this subject, history_hoard is a vector<int> and a member of Food
	int historysize =history_hoard.size();

	sort (history_hoard.begin(), history_hoard.end());

	if (historysize ==0)
		return (0);

	sort (history_hoard.begin(), history_hoard.end());
	
	long left =0;
	for (int i=0; i<historysize; i++) {
		left += history_hoard.at(i);
	}
	
	int mean =0;
	mean = left/historysize;

	int median =0, arg =0;
	double left_double=0;
	
	if (historysize%2 !=0) {
		left_double = 0.5 + (historysize/2);
		arg = left_double; //this is an intended conversion (double to int)
		median = history_hoard.at(arg);
	}
	else if (historysize%2 ==0) {
		int median1=0, median2 =0;
		arg = historysize/2;

		median1 = history_hoard.at(arg);
		median2 = history_hoard.at(arg+1);
		median = (median1 + median2)/2;
	}

	if (optimist ==1) {
		if (median >= mean)
			return (median);
		else
			return (mean);
	}
	else if (optimist ==0) {
		if (median <= mean)
			return (median);
		else
			return (mean);
	}
}

[ne555]

Why are you sorting twice?
It seems that you don't know about integer division:

        left_double = 0.5 + (historysize/2); //Integer division
        arg = left_double;
// When you divide two integers, the result is an integer.
        arg = historysize/2; //it can simplify to this

        int mean =0;
	mean = left/historysize; //mean will be always an integer 

All the control paths reach return statements

[Athar]

The correct way to do this is to use bool, as optimist is a boolean value (even better: enum for self-explaining code).
By the way, you're having quite a lot of superfluous code in there (for example, why sort twice?).
The following, shorter code example shows what I mean:

int Food::get_avg_gather(bool optimist)
{
  const int size=history_hoard.size();
  if (size==0)return 0;

  sort(history_hoard.begin(),history_hoard.end());
  const long sum=accumulate(history_hoard.begin(),history_hoard.end(),0l);

  int mean=sum/size;
  int median;

  if (size%2!=0)median=history_hoard[size/2+1];
  else median=(history_hoard[size/2]+history_hoard[size/2+1])/2;

  if (optimist)return max(median,mean);
  else return min(median,mean);
}

[Aeon221]

Optimist I have to leave alone because I use it in different ways in different places. It's coming from a value two functions and a class away, and I'd rather not rewrite them... buuut I probably should.

The second sort is a copypasta artifact. I decided to move it slightly higher up in the function, used copy instead of cut, and then totally forgot.

I originally had the historysize declared as a const int, but I'm worried it will only initialize the first time the function is called, which would be bad since this is gonna be regularly used. I'm a big fan of const, so I'm hoping I was wrong.

Thanks for accumulate(), I hadn't noticed it -- was looking for sum()!


Huge improvement to the code, so thanks mucho.

[Athar]

I originally had the historysize declared as a const int, but I'm worried it will only initialize the first time the function is called, which would be bad since this is gonna be regularly used.

No, you might be mixing up const with static. const just means that the variable can't be changed, which mainly serves as a hint to the reader.