tags:

views:

108

answers:

4

I'm using g++ on fedora linux 13. I'm just practicing some exercises from my c++ textbook and can't get this one program to compile. Here is the code:

double *MovieData::calcMed() {
        double medianValue;
        double *medValPtr = &medianValue;
        *medValPtr = (sortArray[numStudents-1] / 2);
        return medValPtr;
}

Here is the class declaration:

    class MovieData 
{
private:
    int *students;                  // students points to int, will be dynamically allocated an array of integers.
    int **sortArray;                // A pointer that is pointing to an array of pointers.
    double average;                 // Average movies seen by students.
    double *median;                 // Median value of movies seen by students.
    int *mode;                  // Mode value, or most frequent number of movies seen by students.
    int numStudents;                // Number of students in sample.
    int totalMovies;                // Total number of movies seen by all students in the sample.
    double calcAvg();               // Method which calculates the average number of movies seen.
    double *calcMed();              // Method that calculates the mean value of data.
    int *calcMode();                // Method that calculates the mode of the data.
    int calcTotalMovies();              // Method that calculates the total amount of movies seen.
    void selectSort();              // Sort the Data using selection sort algorithm.
public:
    MovieData(int num, int movies[]);       // constructor
    ~MovieData();                   // destructor
    double getAvg() { return average; }     // returns the average
    double *getMed() { return median; } // returns the mean
    int *getMode()  { return mode; }        // returns the mode
    int getNumStudents() { return numStudents; }    // returns the number of students in sample
};

Here is my constructor and destructor and selectSort():

MovieData::MovieData(int num, int movies[]) {
    numStudents = num;

    // Now I will allocate memory for student and sortArray:
    if(num > 0) {
        students = new int[num];
        sortArray = new int*[num];

        // The arrays will now be initialized:
        for(int index = 0;index < numStudents;index++) {
            students[index] = movies[index];
            sortArray[index] = &students[index];
        }
        selectSort();   // sort the elements of sortArray[] that point to the elements of students.
        totalMovies = calcTotalMovies();
        average = calcAvg();
        median = calcMed();
        mode = calcMode();
    }
}

// Destructor:
// Delete the memory allocated in the constructor.
MovieData::~MovieData() {
    if(numStudents > 0) {
        delete [] students;
        students = 0;
        delete [] sortArray;
        sortArray = 0;
    }
}

// selectSort() 
// performs selection sort algorithm on sortArray[],
// an array of pointers.  Sorted on the values its 
// elements point to.
void MovieData::selectSort() {
    int scan, minIndex;
    int *minElement;

    for(scan = 0;scan < (numStudents - 1);scan++) {
        minIndex = scan;
        minElement = sortArray[scan];
        for(int index = 0;index < numStudents;index++) {
            if(*(sortArray[index]) < *minElement) {
                minElement = sortArray[index];
                minIndex = index;
            }
        }
        sortArray[minIndex] = sortArray[scan];
        sortArray[scan] = minElement;
    }
}

The compiler is giving this error:

moviedata.cpp: In memberfunction 'double * MovieData::calcMed()':

moviedata.cpp:82: error: invalid operands of types 'int*' and 'double' to binary 'operator/'

I'm not sure what to make of this error, i've tried static casting the types with no luck, what does this error message mean?

+2  A: 

sortArray[numStudents - 1] is a pointer to int, which can't be on the left side of a division (when you remember pointers are addresses, this makes sense). If you post more of your code, we can help you correct it.

Perhaps you want something like:

int *MovieData::calcMed() {
      return sortArray[(numStudents - 1) / 2];
}

This returns the middle element in your array, which should be a pointer to the middle student. I'm not clear why you're sorting lists of pointers (not the actual values), or why you're returning a pointer here. The return value + 1 will be a pointer to the next value in students, which is not the next greater value numerically. So you might as well return the actual student (int from students). If you do this, you can also average the two middle elements when the count is even (this rule is part of the typical median algorithm).

Note that I changed the return type to int *, the type of sortArray's elements. Also, your comment is incorrect. This is the median, not the mean.

Also, your selection sort is wrong. The inner loop should start at scan + 1.

Matthew Flaschen
which part of the code would be helpful, the class declaration?
blakejc70
Yes, and where `sortArray` is defined and initialized.
Matthew Flaschen
I added class declaration, constructor and destructor, and the selectSort() member function.
blakejc70
Oops, I didn't realize that I even messed up on a comment, thanks for pointing that out.
blakejc70
Okay, that actually got it to compile, thanks! But I'd still like clarification on that compiler error: It's basically telling me that since my member function was returning a double*, I was trying to return a division between an int* and int? So I should really be returning a int*? Is that right?
blakejc70
so scan+1 should replace index? I'll take a closer look, I learned that from my textbook. can you explain?
blakejc70
You can't divide any pointer by any value. It just doesn't make sense, and isn't allowed. The reason you should be returning an `int *` is that every element in sortArray is an `int *`. If you decided to return the actual median, that would be an int. Yes, `scan + 1` should replace `index`. See this [psuedo-code](http://www.personal.kent.edu/~rmuhamma/Algorithms/MyAlgorithms/Sorting/selectionSort.htm). The reason is that you only want to swap with the smallest *later* element. If you swap with an earlier element that is less, you're undoing your previous sorting work.
Matthew Flaschen
(numStudents - 1)/2 is not right - e.g. will return 1 when numStudents == 4. Technically to calculate the median correctly, you should use numStudents/2 for an odd number of students, and the average of the dereferenced values at numStudents/2 and at (numStudents-1)/2 for an even number of students.
sje397
@sje397, I added a note about that. He would have to return an int, instead of a pointer into `students`.
Matthew Flaschen
+4  A: 

you are trying to divide a pointer by a double, which the compiler is saying it does not know how todo.

sortArray is probably defined by

int ** sortArray;

its also worth noting you are returning a pointer to a stack variable, who's value will be undefined as soon as you return out of the function.

Akusete
what's a stack variable? pardon my ignorance.
blakejc70
The stack is a region of memory used by programming languages to keep track of function calls, function arguments, return values and temporary variables used within functions http://www.masters-of-the-void.com/book5.htm
Akusete
Its also where the term 'stack overflow' is derived from. i.e. when the stack runs out of space and 'overflows' onto the heap or other regions of memory
Akusete
Better links http://en.wikipedia.org/wiki/Stack-based_memory_allocation http://en.wikipedia.org/wiki/Automatic_variable
Akusete
+2  A: 

You probably want something like:

int *MovieData::calcMed() {
    return sortArray[numStudents/2];
}
sje397
note: assuming number of students is odd, for simplicity
sje397
+3  A: 

Your code shows a lack of understanding of pointers. You need to do more reading and practice on simpler examples.

More specifically:

double medianValue; creates a double variable. What for? You're apparently going to return a double * and returning a pointer to a local variable is always wrong, because local variables are "recycled" when their function ends.

double *medValPtr = &medianValue; creates a pointer called medValPtr and sets it to the location of medianValue. Well.

Due to the current contents of medValPtr, *medValPtr = (sortArray[numStudents-1] / 2); has the same effect as typing medianValue = (sortArray[numStudents-1] / 2); (supposing it were to compile at all).

Which it doesn't because sortArray[numStudents-1] is, at a guess, the last item in the array sortArray but happens to be a pointer to something else. You can't divide a pointer (numerically you can, but C++ disallows it's always wrong).

Finally you return medValPtr; which is wrong because medValPtr is pointing to a local variable.

Artelius
Yeah, I'm still trying to learn pointers and dynamic memory allocation. I'm actually doing the exercises from my textbook on the pointers chapter. Are you trying to say that I should just have a return statement instead of all the other garbage?
blakejc70