C++ pointers dynamic arrays and functions

Question

This is from a beginning C++ class, no grade involved since I'm simply following along trying to remember stuff after too many years. The class has reached the point where we're using pointers and dynamic arrays. I'm trying to pass the array (or the pointer) to different functions having to do with various calculations on a list of integers. Some of the functions are working and others aren't. But I'm passing them all the same way and I'm calling them all the same way as well. Since some of the functions are returning garbage values, it feels like I'm not actually pointing where I think I am AND the functions that are working are only working by accident.

Before someone helpfully suggests that I should be using vectors and not arrays, the point of the exercise is to use dynamically allocated arrays.

Here's my code so far. It's the findavg() and findmedian() that are returning garbage.

#include <iostream>
#include <iomanip>

using namespace std;

void getarray(int *a, int &l)
{
    int input = 0;

    //-1 is not important here, just to make the while loop run
    while(input != '-1') 
    {
        cout << "Enter length of the list of numbers: ";

        if(!(cin >> input))
        {
            cout << "Please enter numeric characters only." << "\n";    
            //show error if they don't enter a number
            cin.clear();                                                
            //get rid of invalid characters
            cin.ignore(10000,'\n');
        }

        else if(input <= 0)
        {
            cout << "Please enter a number greater than 0." << "\n";    
            //show error if they enter a non-positive number
            cin.clear();                                                
            //get rid of invalid characters
            cin.ignore(10000,'\n');
        }
        else
        {
            l = input;  //input is a positive number
            break; //got good input, break out of while loop
        }

    }


    int i;
    int x; 

    cout << "Enter " << l << " integers on one line seperated by spaces.\n";


    for( i = 0; i < l  &&  cin >> x; ++i)
    {
        a[i] = x;
    }

}

void printarray(int *a, int &l)
{
    int i;

    cout << "The array of integers is:\n";
    for( i = 0; i < l; ++i)
        cout << setw(4) << a[i];
    cout << "\n";

}

int findmin(int *a, int &l)
{
    int min = 0;
    min = a[0]; 

    for(int i = 1; i<l; i++)
    {
        if(a[i] < min)
            min = a[i];
    }
    return min; 
}

int findmax(int *a, int &l)
{
    int max = 0;
    max = a[0];

    for(int i = 1; i<l; i++)
    {
        if(a[i] > max)
            max = a[i];
    }
    return max; 
}

float findavg(int *a, int &l)
{
    int total = 0;
    total = a[0];

    for(int i = 1; i<l; i++)
    {
        total = total + a[i];
    }

    return static_cast<float>(total/static_cast<float>(l));

}

float findmedian(int *a, int &l)
{
    int max = 0;
    int min = 0;

    max = findmax(a, l);
    min = findmin(a, l);

    return static_cast<float>((max + min)/2.0);

}


int main()
{

    int length = 0;
    int *an_array;

    an_array = new int[length];

    getarray(an_array, length);

    printarray(an_array, length);

    cout << "Maximum value in list is: "<< findmax(an_array, length) << "\n";
    cout << "Minimum value in list is: "<< findmin(an_array, length) << "\n";
    printf("Average is: %10.5f", findavg(an_array, length));
    cout << "\n";
    printf("Median is: %10.5f", findmedian(an_array, length));
    cout << "\n";

    delete [] an_array;

    return 0;
}

Did you learn about std::vector et. al. yet or did your professor start with dynamic arrays? — Xeo
– Xeo, Commented Nov 30, 2011 at 7:47
length is always 0 when allocating memory for array!! You need to allocate memory in getarray — another.anon.coward
– another.anon.coward, Commented Nov 30, 2011 at 7:51
Already did an exercise with std::vector. For this drill we've been told specifically to NOT use vectors. — John McDermon
– John McDermon, Commented Nov 30, 2011 at 7:52
@John: Good, atleast your C++ course isn't rubbish, as many just start with dynamic arrays et. al. before even showing the standard library. :) — Xeo
– Xeo, Commented Nov 30, 2011 at 7:54

Sarfaraz Nawaz · Accepted Answer · 2011-11-30 08:27:57Z

1

int length = 0;
int *an_array;

an_array = new int[length]; //What is this ?

You're allocating zero bytes for an_array, because length is 0. That is one problem I can see.

You should allocate memory for an_array in getarray function, after reading the length of array:

void getarray(int * &a, int &l) //see the change here!
{
    //code which reads length is omitted for brevity

    a = new int[l]; //alllocate memory after reading length

    //now read array elements here
}

Alternatively,you can write a struct as:

struct array
{
      int *data;
      size_t size;
};

And then use this everywhere. It is better because you tied the size and data together, and instead of using two independent objects, you can use just one object of type array, as described below.

array getarray() 
{
    array arr;

    //read length into arr.size

    arr.data= new int[arr.size]; //alllocate memory after reading length

    //now read array elements into arr.data

    return arr; //returning arr means you're returning both: data and size!
}

And then call this as:

 array arr = getarray(); //no need to pass any argument!

Also, if you're going to use this, then other functions signature would become:

void printarray(array arr);
int findmax(array arr);
float findavg(array arr);
float findmedian(array arr);

Now you need to modify these functions as well, but fortunately, there will not be major change : wherever you're using a and l, use arr.data and arr.size respectively instead. And you're done!

With all these improvements, your main() would become this:

int main()
{
    array arr = getarray();

    printarray(arr);

    cout << "Maximum value in list is: "<< findmax(arr) << "\n";
    cout << "Minimum value in list is: "<< findmin(arr) << "\n";

    printf("Average is: %10.5f\n", findavg(arr));
    printf("Median is: %10.5f\n", findmedian(arr));

    delete [] arr.data;
    return 0;
}

This looks better.

edited Nov 30, 2011 at 8:27

answered Nov 30, 2011 at 7:55

Sarfaraz Nawaz

363k121 gold badges682 silver badges866 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

John McDermon Over a year ago

Thanks for the tip -- combining your points with everyone else's comments should get me there.

Sarfaraz Nawaz Over a year ago

@JohnMcDermon: Now, see how main() looks like!

John McDermon Over a year ago

I like it. I'll have to give it a try and see how it affects things when I implement the correct getmedian() and then the sort() functions. Thanks!

Sarfaraz Nawaz Over a year ago

@JohnMcDermon: There is no major change in those functions as well. For example, wherever you're using a and l, use arr.data and arr.size respectively instead.

another.anon.coward Over a year ago

@Nawaz: Do you think it will be a good idea to allocate & release memory as part of ctor & dtor of the structure so that its ok to forget delete [] in the main()? Or am I missing something in regard to this?

|

David Heffernan · Accepted Answer · 2011-11-30 08:15:37Z

0

You are not allocating any memory for the array. Undefined behaviour ensues.

Change your getarray function to do the allocate and return the newly created and populated array:

int* getarray(int &length)

or even

void AllocateAndPopulateArray(int* &array, int &length)

for a more symmetrical interface.

If you want a non-terminating loop use

while(true)

There is no need to bring input into your while loop.

There's no point passing the length by reference in any of the other routines since you are not modifying it. In fact you are asking for trouble passing by reference since a coding error may inadvertently change the value. You should declare it with const to make the compiler catch any silly mistakes you might make.

I most heartily agree with Nawaz's suggestion to tie together the pointer and length variables.

In your average calculation I personally would declare total to be a float and thus avoid the awkward looking casts.

Your implementation of median is plain wrong, but that seems to be a side issue.

edited Nov 30, 2011 at 8:15

answered Nov 30, 2011 at 7:51

David Heffernan

616k46 gold badges1.1k silver badges1.5k bronze badges

3 Comments

John McDermon Over a year ago

Thanks David I see everything you pointed out. Certainly feel foolish about the error in getmedian() -- it's been too long :( One thing, when I wasn't using the static_cast I was only getting integer results for getavg(). Not sure why, if the compiler is supposed to promote it to a float -- maybe it didn't since l is an int and I can't add a decimal place to induce floating point math without the cast?

David Heffernan Over a year ago

That's my mistake. I misread in a hurry. I assumed total was a float. That's how I would have done it. Should not have assumed.

John McDermon Over a year ago

Ahhhh, but you bring up a good point -- I should declare total as a float and then I'd avoid the cast. I like it :)

Collectives™ on Stack Overflow

C++ pointers dynamic arrays and functions

2 Answers 2

7 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

7 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related