pointer and memory allocation trouble

Code:

#include <iostream>
using namespace std;

int   *allocate          (int number);

float average(int *array, int number);

void  sort   (int *array, int number);

int main()
{
	int number;

	int *p;

	cout << "How many test score do you want to average?" << endl;

	cin  >> number;

	while (allocate(number) == NULL)
	{
		cin.clear();
		cin.ignore();

		cout << "There was an error in allocating elements" << endl;
		cout << "Please try again." << endl;
		cin  >> number;
	}

	p = allocate(number);

	cout << "\nEnter the test scores" << endl;

	for (int i = 0; i < number; i++)
	{
		cout << "Test score" << " #" << *(p + i + 1);
		cin  >> *(p + i);

		if (cin.fail() || *(p + i) < 0)
		{
			cout << "Invalid entry" << endl;	
			cout << "Re-enter" << endl;

			cout << "Test score" << " # " << *(p + i + 1);
			cin  >> *(p + i);
		}
	}

	sort(p, number);

	cout << "\nHere are the scores you entered in sorted form:" << endl;

	for (i = 0; i < number; i++)
		cout << *(p + i) << endl;

	cout << "\nThe average of the scores is " << average(p, number) << endl;

	return 0;
}

int *allocate(int number)
{
	int *array;

	array = new int[number];

	return array;

	delete [] array;
}

float average(int *array, int number)
{
	float sum = 0.0f;

	for (int i = 0; i < number; i++)
{
	sum += *array++;
}

	return sum / number;
}

void  sort   (int *array, int number)
{
	int hold;
	int finish;

	do
	{
		finish = 0;

		for (int i = 0; i < number - 1; i++)
		{	
			if (*(array + i) > *(array + i + 1))
			{
				hold             = *(array + i);
				*(array + i)     = *(array + i + 1);
				*(array + i + 1) = hold;

				int finish = 1;
			}
		}
	}while (finish != 0);
}

The cout statements in the first for loop are printing out strange values.

I'm not sure what's wrong with this:

Code:

cout << "Test score" << " #" << *(p + i + 1);

Also, did I put delete [] array; in the right place?

My God....that code...it's in code tags...and still has no indentation and crazy spacing. Any chance you could edit that?

I don't know why you have a function for allocation like that, but if you insist, this is what it should look like:

Code:

int* 	allocate(unsigned int number) {

	return new int[number];
}

Line 36 of your program should look like this:

Code:

cout <<"Test score #" <<i + 1 <<": ";

In line 53, you need to declare i for the for loop(i was declared in a previous for loop, but that was local to that for loop), like this:

Code:

for(int i = 0;i < number;i++)

Right before you return 0 from main, you need to delete that allocated integer array like this:

Code:

delete [] p;

With those changes made, the program should work alright. Hope that helps. BTW, be careful with that pointer arithmetic;

Code:

int* 	allocate(unsigned int number) {

	return new int[number];
}

Why did you change the value in the function's parameter to unsigned? Is it because of that if statement in main?

Code:

if (cin.fail() || *(p + i) < 0)

Unsigned integers are positive numbers, so I guess that makes sense.

In line 53, you need to declare i for the for loop(i was declared in a previous for loop, but that was local to that for loop), like this:

Code:

for(int i = 0;i < number;i++)

I'm afraid you're wrong. The i that was declared in the previous for loop is local to main - not the for loop itself.

Right before you return 0 from main, you need to delete that allocated integer array like this:

Code:

delete [] p;

What about that other one?

Code:

delete [] array;

I'm still not sure where to delete those in my program.

What about that other one?
I'm still not sure where to delete those in my program.

How about at the end of main (before return)?
You deallocate the memory when you don't need it any longer.

I'm afraid you're wrong. The i that was declared in the previous for loop is local to main - not the for loop itself.

When you declare a variable in the for statement like that, you are making it local to only that loop. The variable SHOULD be destructed after that loop and is no longer accessible. If you want to make it global throughout main, then declare it in your main and not in the loop.

Originally posted by HaLCy0n
When you declare a variable in the for statement like that, you are making it local to only that loop.

If that were true, my code wouldn't work, right? Wouldn't I receive an error or at least a warning?

Variables are stored in memory, so unecessarily declaring them is wasting memory, right?

Originally posted by PorkyChop

In line 53, you need to declare i for the for loop(i was declared in a previous for loop, but that was local to that for loop), like this:

Code:

for(int i = 0;i < number;i++)

When I did that, I actually got an error.

Code:

error C2374: 'i' : redefinition; multiple initialization

Also, is there something wrong with my sorting algorithm? It's not sorting the numbers properly.

>When I did that, I actually got an error.

>code:--------------------------------------------------------------------------------
>error C2374: 'i' : redefinition; multiple initialization


If you have a newer compiler, the variable is only local to the loop, in accordance with the latest standard I would presume.

Originally posted by swoopy
>When I did that, I actually got an error.

>code:--------------------------------------------------------------------------------
>error C2374: 'i' : redefinition; multiple initialization


If you have a newer compiler, the variable is only local to the loop, in accordance with the latest standard I would presume.

I'm using VC++ 6.0. And does anyone know the real deal concerning this i issue?

I really don't care, just as long as my code works. Speaking of that, what's wrong with my sorting algorithm?

Originally posted by volk
I'm using VC++ 6.0. And does anyone know the real deal concerning this i issue?

I really don't care, just as long as my code works. Speaking of that, what's wrong with my sorting algorithm?

You should care, because don't take it for granted. A lot of compilers will shoot errors at you for that, and it could be a hard bug to find if it does compile. It technically is local to the for loop. just don't take it for granted because it won't always work.

rethink your sorting algorithm. take a look at your variable declarations first. then try to figure out what's wrong from there. plus, what exactly is the problem with it?

You have to enable a compiler option to make variable declarations in for-statements work as intended.

Code:

	while (allocate(number) == NULL)
	{
		cin.clear();
		cin.ignore();

		cout << "There was an error in allocating elements" << endl;
		cout << "Please try again." << endl;
		cin  >> number;
	}

	p = allocate(number);

This is flawed. You allocate memory. If the allocation fails, you try again. If it succeeds you let the pointer hang around (memory leak!!!!) and allocate more memory, but you don't check for success on that.

It should be

Code:

	while ((p = allocate(number)) == NULL)
	{
		cin.clear();
		cin.ignore();

		cout << "There was an error in allocating elements" << endl;
		cout << "Please try again." << endl;
		cin  >> number;
	}

Except that the allocate function is useless.

Originally posted by alpha
plus, what exactly is the problem with it?

If I enter 10, 6, 5, 1, 3, it's sorted in this order: 6, 5, 1, 3, 10

Originally posted by CornedBee

Except that the allocate function is useless.

Originally posted by volk
Except that the allocate function is useless.

You made a function that allocates memory, while you could just call new directly.

As for the for-loop thing, just declare the loop integer outside and you don't have to care anymore .

Code:

int i;

for(i=0; i<5; i++)
{
   ...
}

for(i=8; i>0; i--)
{
   ...
}

etc...