Finding the Mode - Range Error

[Theo Berlin]

Hello, I'm trying to make a program that inputs positive integers and finds the mode. It may be unnecessarily long and complex, but here it is.

#include "../../std_lib_facilities.h"
int main()
{
	vector<int>i;
	vector<int>repeated_numbers;
	int integers = 1;
	int number;
	int occurances = 0;
	int max_number = -1;
	int max_occurances = 0;
	cout << "Enter positive integers. Enter 0 to get the mode.\n";
	while (cin >> integers)
	{
		if (integers < 1)
			break;
		else
		i.push_back(integers);
	}
	cout << "1";
	sort (i.begin(), i.end());
	cout << "2" << endl;
	for (size_t v= 1; v < i.size(); ++v)
	{
		cout << "3	";
		if (i[v-1] == i[v]){
			repeated_numbers.push_back(i[v-1]);
			repeated_numbers.push_back(i[v]);
		}
		cout << "v=" << v;
		cout << "	4, i(v)=" << i[v] << endl;
	}
	cout << "5";
	sort (repeated_numbers.begin(),repeated_numbers.end());
	cout << "6";
	for (size_t i=0; i < repeated_numbers.size()-1; ++i){
		occurances = 0;
		cout << "7";
		number = repeated_numbers[i];
		cout << "number=" << number << endl;
		if (repeated_numbers[i] != max_number){
		cout << "8, max_number=" << max_number;
		for (int v = 1; repeated_numbers[i+v] == repeated_numbers[i]; ++v){
			occurances += 1;
			cout << "	9, v=" << v << endl;
			if (occurances > max_occurances)
				{
				cout << "10, occurances=" << occurances;
				max_occurances = occurances;
				max_number = number;
				cout << "	max_number=" << max_number << endl;
				}
			}
		}
	}
	cout << "The mode is " << max_number << endl;
	keep_window_open();
}

All the "cout << (number)" are just for testing. Basicly, the program (almost) does what it should. When I break the first loop using ctrl+z or break;, I get a message saying things like "unhandled exception..." and "...range_error...". If I continue I get an error message saying "debug assertion failed..." and "Vector subscript out of range".

Any idea what's causing this? Am I reading an element that doesn't exist or anything?

Thanks!
Theo

[vlad from moscow]

This loop

for (size_t i=0; i < repeated_numbers.size()-1; ++i){

is invalid. Vector repeated_numbers can be empty. In this case size() is equal to 0 and size() - 1 is equal to the maximum value of the corresponding unsigned type that size() has.

[Theo Berlin]

size()-1 was an experiment, -1 has been removed. The program doesn't work even if I have repeated_numbers.push_back() at the beginning. This line ensures that even if the user doesn't input repeated numbers, there's still an element.

Vector repeated_numbers can be empty.

Are you sure that's causing it?

[vlad from moscow]

I simply pointed out one bug in your program. Your code is so bad that there is no any desire to investigate it.

[Theo Berlin]

That felt like a very disrespectful, unnecessary kick in the butt. I've been programming for less than a week and I know my codes are dumb. However, I rewrote the program, is this more worthwhile?

#include "../../std_lib_facilities.h"
int main()
{
	int occ = 0;
	int max_occ = 0;
	int num;
	int max_num;
	int integers;
	vector<int>array;

	while (cin >> integers)
	{
		if (integers == 0)
			break;
		else 
			array.push_back(integers);
	}
	sort(array.begin(),array.end());
	for (size_t i = 0; i < array.size(); ++i)
	{
		int j = i + 1;
	if (array[i] == array[j])
	{
		num = array[i];
	occ += 1;

	if (occ > max_occ)
	{
		max_occ = occ;
		max_num = num;
	}
	}
	else if (array[i] != array[j])
		occ = 0;
	}
	cout << "The mode is " << max_num << endl;
	keep_window_open();
}

I know it has the same problem, array.size might be empty. But this isn't intended to be used my the public.

Anyways, the reason why I posted this new one is because the compiler (microsoft visual studio) is giving the the exact same message. As soon as I break the first while loop, it says the same things (unhandled exception, vector subscribt out of range etc). Any idea what it might be?

[vlad from moscow]

Let consider your code from the very beginning

	int occ = 0;
	int max_occ = 0;
	int num;
	int max_num;
	int integers;
	vector<int>array;

	while (cin >> integers)
	{
		if (integers == 0)
			break;
		else 
			array.push_back(integers);
	}

The definitions of variables say nothing to the reader because he does not understand their meaning before seeing their usage. Also it is a bad idea to name an object of type vector as array. It would be much better to name it at least v. So the code should start the following way

	int integers;
	vector<int> v;

	while ( cin >> integers && integers != 0 ) v.push_back( integers );

Also I do not like name integers because it is not clear why one object has a name in plural. I would write this code snip the following way

	int number;
	vector<int> v;

	while ( cin >> number && number != 0 ) v.push_back( number );

You should introduce new names in the code only when they are used immediately. Otherwise it is difficultly to read code.

[cire]

That felt like a very disrespectful, unnecessary kick in the butt.

Vlad can be a bit gruff. Nevertheless, people are far less likely to poke into a mess like your original code than they are into something which uses whitespace and indentation appropriately (and consistently) to increase the readability of code. (Something your last contribution improves, but still falls short on.)

	for (size_t i = 0; i < array.size(); ++i)
	{
		int j = i + 1;

We know from the loop condition and i's initial value that i will always be a valid index into array. Can the same be said of i + 1?

[vlad from moscow]

By the way you simply substituted one bug for another bug.

	for (size_t i = 0; i < array.size(); ++i)
	{
		int j = i + 1;
	if (array[i] == array[j])

if i is equal to size() -1 then j is equal to size(). However there is no such an element with index equal to size() in the vector (except when the vector is empty).