C++ Problem Solving

The FindMax() function given below is used to find the maximum value in an array of integers which has a start address in memory defined by the pointer pVals. The number of elements in the array is defined by the parameter NumEls. This function compiles and runs but the produces an incorrect result.

What is wrong with the function?

Code:

---

int FindMax(int * pVals, int NumEls)
{
        int i, Max = *pVals++;
       
        for (i=1; i<NumEls; ++i)
        {
                          if(Max < *pVals++)
                        Max == *pVals;
                }
        return Max;
}

---

What do you think is wrong with it?

well why don't you try going through the function and seeing whats going on. It's pretty easy to see if you look at it well enough.

Turn up the warning level on your compiler. You should be warned about this.

Quote:

---

Originally Posted by scojan

What is wrong with the function?

---

Two things... One of them is, what happens when NumEls is 0? The other will be revealed if you actually listen to the compiler warnings.

You could also save time and just use std::max_element()

Actually, 3 things are wrong, and I don't think the third will be found by turning up compiler warnings. However, it will be found if you just think through the code step by step.

Defining ‘i = 1’ in the line:

Code:

---

for(i=1; i<NumEls; ++i)

---

Missing the first entry of the array out each time the function is run (first element of an array is defaulted as 0).

Code:

---

Max == *pVals;

---

"warning C4553: '==' : operator has no effect; did you intend '='?"

This is what I have so far.

Trying to determine the problems of original problem by using the following code:

Code:

---

#include "stdafx.h"
#include <iostream>
using namespace std;



int main()
{
int * pVals;
int NumEls[5];
pVals = NumEls;
*pVals = 10;
pVals++;  *pVals = 20;
 pVals = &NumEls[2];  *pVals = 30;
 pVals = NumEls + 3;  *pVals = 40;
 pVals = NumEls;  *(pVals+4) = 50;

        int i, Max = *pVals++;

        for (i=0; i<5; ++i)
cout << NumEls[i] << ", ";       
        {
                        if(Max < *pVals++)
                        Max == *pVals;
        }
cout << Max;
return Max;
}

---

>> Defining ‘i = 1’ in the line: for(i=1; i<NumEls; ++i)
Nothing "wrong" with this, it doesn't define i it initializes it. Although I would have actually defined i there: for(int i=1; i<NumEls; ++i)

>> Missing the first entry of the array out each time the function is run
Is it? Look again.

>> "warning C4553: '==' : operator has no effect; did you intend '='?"
Do you know what that means?

The other issue is this:

Code:

---

if(Max < *pVals++)
    Max = *pVals;

---

You check the current Max value with the value in the array and if you need to update Max you update it with the next value (not the one you just compared against) since the pointer has already been incremented by the time you reach the assignment. If the last element in the array happens to be the highest value (as an example), then you set Max to whatever value is in the next slot (an out-of-bounds memory location) which could be anything, even a really low value. You should not be doing the increment of the pointer there.

Code:

---

#include "stdafx.h"
#include <iostream>
using namespace std;

int main ()
{
  int NumEls[5] = {15, 25, 40, 50, 24};
  int * pVals;
  pVals = NumEls;
  int max = *pVals++;
 
 

  for (int i = 0; i<5; i++)
    {
      if (max < *pVals)
       
          max = *pVals;
     
    }
  cout << "Maximum element in the array :  " << max;

  return 0;
}

---

It is giving me the maximum value as being 25, what have I done wrong. When I change these values it is always giving the second integer in the array as the maximum value.

You do still have to increment pVals at some point... Otherwise you are just getting the first value always.

--
Mats

Now you don't increment the pointer at all, so the only values you ever check is the first and the second.

Why are you messing with pVals ?
You increment it in some places, and fail to increment it in other places.

Try

Code:

---

max = NumEls[0];
for ( i = 1 ; i < 5 ; i++ ) {
  if ( max < NumEls[i] ) max = NumEls[i];
}

---

Yes you can use the pointer, and increment it with ++, but you need to be rather careful about when you actually increment it.

Code:

---

      if (max < *pVals++)
          max = *pVals++;

---

Trying to use ++ in either of these places for example would be wrong.

Quote:

---

Originally Posted by Salem

Why are you messing with pVals ?
You increment it in some places, and fail to increment it in other places.

Try

Code:

---

max = NumEls[0];
for ( i = 1 ; i < 5 ; i++ ) {
  if ( max < NumEls[i] ) max = NumEls[i];
}

---

---

I agree. And for a simple example like this, the compiler will most likely generate at least as good code for this as for the pointer example [assuming the pointer was introduced in an attempt to "make it better"]. The above is also more obvious for anyone else reading the code.

--
Mats

Yeah its working now, thanks a lot for help - even if it was probably one of the most basic problems you've seen on these boards xD

Code:

---

#include "stdafx.h"
#include <iostream>
using namespace std;

int main ()
{
  int NumEls[5] = {23, 54, 29, 35, 40};
  int * pVals;
  pVals = NumEls;
  int max = NumEls[0];
       
        for (int i = 1 ; i < 5 ; i++ )
        {
                if (max < NumEls[i])
                max = NumEls[i];
        }
 
        cout << "Maximum element in the array :  " << max;

  return 0;
}

---

Quote:

---

Originally Posted by scojan

one of the most basic problems you've seen on these boards xD

---

Not by far.