finding max and min integers - newbie question

[john]

Hi

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

Any help would be appreciated.

John

 

[Hendrik Belitz]

Hi

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

That's simple:

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

 

[David Fisher]

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

Assuming they are already in a non-empty vector (std::vector<int>) called
"vec":

#include <algorithm>

....

int minValue = *(std::min_element(vec.begin(), vec.end()));
int maxValue = *(std::min_element(vec.begin(), vec.end()));

This takes about twice as long as finding them both simultaneously (as in
Hendrik's solution), but it is simple ...

David F

 

[David Fisher]

Assuming they are already in a non-empty vector (std::vector<int>) called
"vec":

#include <algorithm>

...

int minValue = *(std::min_element(vec.begin(), vec.end()));
int maxValue = *(std::min_element(vec.begin(), vec.end()));

Doh ! Spot the deliberate mistake ...

David F

 

[Karl Heinz Buchegger]

Hi

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

That's simple:

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

Not quite (But the modification is trivial and left
as an exercise for the OP)!

 

[Hendrik Belitz]

Hi

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

That's simple:

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

No, it won't:

int arr[] = { 5 };

min -> 5
max -> 5

which is correct

 

[Karl Heinz Buchegger]

john wrote:

Hi

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

That's simple:

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

No, it won't:

int arr[] = { 5 };

min -> 5
max -> 5

which is correct

Run the program and try it!
In the above max will never be set, since there
is an else in it!

 

[Kamil Burzynski]

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

No, it won't:

Run the program and try it!
In the above max will never be set, since there
is an else in it!

Actually there is too much else in code Removing 'else' statement
from code above will remove bug.

 

[Karl Heinz Buchegger]

Hendrik Belitz wrote:

john wrote:

Hi

I'm a learner and I'm trying to work out the simplest way of finding
the maximum and minimum value from a group of integers.

That's simple:

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;


What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

No, it won't:

int arr[] = { 5 };

min -> 5
max -> 5

Wrong. But I am wrong too. It gives

min -> 5
max -> INT_MIN

 

[Chris Theis]

Despite its simplicity it turns out to be tricky

int arr[arraySize};

Here we've got a typo!

int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

You are lacking the definition of i!

What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

No, it won't:

int arr[] = { 5 };

min -> 5
max -> 5

which is correct

Yes, this is correct but it is not what your program delivers. If it does
then I'd suggest to get rid of the compiler you are using. The result is

min -> 5
max -> INT_MIN

due to the superficial else. Although removing the else only will not be
sufficient, for in that case you will introduce another bug due to the lack
of missing curly brackets in the for loop.

The correct solution is:

const int arraySize = 1;
int arr[arraySize] = {5};
int min = INT_MAX; int max = INT_MIN;
for( int i = 0; i < arraySize; ++i ) {
if ( arr < min ) min = arr;
if ( arr > max ) max = arr;
}

Or simply write

for ( int i = 0; i < arraySize; arr < min ? min = arr : i = i, arr

max ? max = arr[i++] : i++ ) {}

Regards
Chris

 

[Chris Theis]

int arr[arraySize};
int min = INT_MAX; int max = INT_MIN;
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

What would your code give in the case of:

int arr[] = { 5 };

min -> 5
max -> INT_MAX

No, it won't:

Run the program and try it!
In the above max will never be set, since there
is an else in it!

Actually there is too much else in code Removing 'else' statement
from code above will remove bug.

Unfortunately this will only replace one bug by another due to missing curly
brackets. See my other post.

Regards
Chris

 

[Jeff Schwab]

Doh ! Spot the deliberate mistake ...

David F

You are forgiven, since your STL-based solution is very neat.

 

[Chris Theis]

Assuming they are already in a non-empty vector (std::vector<int>) called
"vec":

#include <algorithm>

...

int minValue = *(std::min_element(vec.begin(), vec.end()));
int maxValue = *(std::min_element(vec.begin(), vec.end()));

I guess you mean *(std::max_element(vec.begin(), vec.end())); here

This takes about twice as long as finding them both simultaneously (as in
Hendrik's solution), but it is simple ...

Why not sort the vector and thus you have the min & the max value at once?

Regards
Chris

 

[Kamil Burzynski]

Hendrik Belitz wrote:
int arr[arraySize};
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

Actually there is too much else in code Removing 'else' statement
from code above will remove bug.

Unfortunately this will only replace one bug by another due to missing curly
brackets. See my other post.

Are you referring to a typo in declaration of array?

Apart from that I dont see the way that bugs may be _replaced_ here.. of course
additional (superfluous) curly brackets would be welcome after if, but
in this code there is no bugs related to it (i.e. after else removed).
At least I don't see them

 

[Chris Theis]

int arr[arraySize};
for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;
Actually there is too much else in code Removing 'else' statement
from code above will remove bug.

Unfortunately this will only replace one bug by another due to missing curly
brackets. See my other post.

Are you referring to a typo in declaration of array?

Apart from that I dont see the way that bugs may be _replaced_ here.. of course
additional (superfluous) curly brackets would be welcome after if, but
in this code there is no bugs related to it (i.e. after else removed).
At least I don't see them

Changing the code from

for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
else if ( arr > max ) max = arr;

to

for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;
if ( arr > max ) max = arr;

is equivalent to the following (with better formatting to illustrate what
I'm aiming at)

for ( i = 0; i < arraySize; i++ )
if ( arr < min ) min = arr;

if ( arr > max ) max = arr;

The second if statement is not included in the loop as their is no
indication of an execution block. Hence, it will be executed only once after
the loop has finished and this is certainly not what we want

Best regards
Chris

 

[Kamil Burzynski]

Why not sort the vector and thus you have the min & the max value at once?

This may be solution sometimes, but generally it is bad idea:
- if you put such code to function like find_min_max() - nobody will
expect that this function will modify this vector, and it may get
const argument
- if you will copy instead of in-place sort it can eat much memory
- sorting is much slower than just reading all values once (and 2
comparisons)
- etc.

IMHO worst of all is that you will modify original vector, which may be
not acceptable.

 

[Jeff Schwab]

I guess you mean *(std::max_element(vec.begin(), vec.end())); here




Why not sort the vector and thus you have the min & the max value at once?

Regards
Chris

"At once?" Why would one replace an O(N) operation with an O(N log N)
operation requiring more storage?

 

[Karl Heinz Buchegger]

Are you referring to a typo in declaration of array?

No.
Original code (reformatted to emphasize the problem)

for ( i = 0; i < arraySize; i++ )
if ( arr < min )
min = arr;
else
if ( arr > max )
max = arr;

One can clearly see now, that the second if is only executed
in case arr is not a new minumum. That means: if the
array only holds a minimum (as was the case with the test
case I provided), max will never be set.
(I know that you have catched that problem. Nevertheless
I wanted to demonstrate how simple errors can crop into
a source code if one doesn't pay attention to the simplest
of all rules: write neat code and don't squeeze everything
in one or two single lines! Formatting and indentation
are your friends! )

Now drop the else, as you suggested (again reformatted)

for ( i = 0; i < arraySize; i++ )
if ( arr < min )
min = arr;

if ( arr > max )
max = arr;


See it now?

Apart from that I dont see the way that bugs may be _replaced_ here.. of course
additional (superfluous) curly brackets would be welcome after if, but
in this code there is no bugs related to it (i.e. after else removed).

There are. You want the second if to be under the control of the for
loop. Without the curly brackets, it won't.

 

[Kamil Burzynski]

Kamil Burzynski wrote:
Now drop the else, as you suggested (again reformatted)

for ( i = 0; i < arraySize; i++ )
if ( arr < min )
min = arr;

if ( arr > max )
max = arr;


See it now?

There are. You want the second if to be under the control of the for
loop. Without the curly brackets, it won't.

Ah, yes, I didnt saw it. Thats the reason I always put brackets, too
Long time ago I didnt.. but after wasting 2 days hunting such stupid bug
I've learned it

 

[Chris Theis]

[SNIP]

Ah, yes, I didnt saw it. Thats the reason I always put brackets, too
Long time ago I didnt.. but after wasting 2 days hunting such stupid bug
I've learned it

Welcome to the club. I guess that's a trap everybody has to fall into.

Cheers
Chris