3
\$\begingroup\$

Is this the most robust and fastest way for finding the min and max out of an array without using STL functions? How can I improve it?

#include <stdio.h>
#include <stdlib.h>
int main(int argc, char const *argv[]) {
system("clear");
int c=9,n[10],max,min,i;
while (c>=0) {
n[c]=random()%100;
printf("%d\n",n[c]);
c--;}c=10;
max=n[0];min=n[1];
for ( i =0; i<c; i++) {
  if(max<n[i])max=n[i];
  else if(min>n[i])min=n[i];
}
printf("\n%d\n%d\n",max,min);
return 0;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Is this how it's formatted in your IDE? There's hardly any indentation here. \$\endgroup\$ – Jamal Oct 13 '15 at 2:18
  • \$\begingroup\$ Your edit is incorrect. You're re-using i for both the inner and outer for loops. \$\endgroup\$ – user1118321 Oct 13 '15 at 4:40
  • \$\begingroup\$ Yep . I am . What's the problem with it ? Is it decreasing performance ? Is it eating up a lot of memory ? @user1118321 \$\endgroup\$ – Italian Oct 13 '15 at 4:44
  • \$\begingroup\$ It doesn't work correctly. The code sets i to 0, sets the min and max values, then sets i to 0 again, and through the course of the loop increments it up to SIZE. When the inner loop finishes the first time, the outer loop will finish, too. Is that what you intended? If so, why do you have the outer loop? \$\endgroup\$ – user1118321 Oct 13 '15 at 4:46
  • \$\begingroup\$ Thanks mate . I was being stubborn , sorry for the obnoxious comment @user1118321 \$\endgroup\$ – Italian Oct 13 '15 at 4:50
3
\$\begingroup\$

First of all, format your code. Indentation goes along way. If everything is left-justified, you can't make out any of the logic.

Secondly, this is a good idea:

max=n[0];min=n[1];

But we should really initialize both to n[0], and on separate lines:

max = n[0];
min = n[0];

The reason is this allows us to start at index 1:

for (i=1; i<c; ++i) {
    if (n[i] > max) {
        max = n[i];
    }
    else if (n[i] < min) {
        min = n[i];
    }
}

We could also consider putting this into a function:

void minmax(int* arr, int len, int* min, int* max) { ... }

minmax(n, c, &min, &max);

Any time you're doing something k times for some constant, or filling in an array - do it front to back. It will be much easier to understand. Don't fill n from index 9 to 0... fill it from 0 to 9. Also, prefer a for loop to a while loop and use i both times:

int c=10, n[10];
for (i=0; i<c; ++i) {
}

And lastly, c is a bad name for the max size of your array. Something like SIZE or LENGTH or anything to that effect would be better.

\$\endgroup\$
  • \$\begingroup\$ I agree with your points, but on the last one, I'd argue that SIZE and LENGTH are only slightly less meaningless than c. Size of what? Length of what? In what units? Perhaps NUM_VALUES? (I also hate value, but this is an exercise, so the values have no real meaning. If they were grades, or dollar amounts, or something, I'd suggest NUM_GRADES, or NUM_DOLLARS or something like that.) \$\endgroup\$ – user1118321 Oct 13 '15 at 4:39
1
\$\begingroup\$

Is this the most robust and fastest way for finding the min and max out of an array without using STL functions?

You tagged your question , but STL is a part of C++.

Your random() is not really random. See what man random says:

If no seed value is provided, the random() function is automatically seeded with a value of 1.

You didn't provide a seed so random() returns the same numbers at every run.

You don't use int argc, char const *argv[] parameters and you can safely remove them. Both clang and gcc inform about this with -Wextra flag:

main.c:3:14: warning: unused parameter ‘argc’ [-Wunused-parameter]
main.c:3:32: warning: unused parameter ‘argv’ [-Wunused-parameter]
\$\endgroup\$
0
\$\begingroup\$

the following code incorporates the comments,

cleanly compiles

performs the desired actions

and is about a quick as I know how to do it.

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

#define MAX_VALUES (10)

int main( void )
{
    system("clear");


    int n[MAX_VALUES];
    int max;
    int min;
    int i;

    srandom( time(NULL) );

    for( i = 0; i < MAX_VALUES; i++ )
    {
        n[i] = random()%100;
    }


    max=n[0];
    min=n[0];

    for ( i=1; i<MAX_VALUES; i++)
    {
        if(max<n[i]) max=n[i];
        else if(min>n[i]) min=n[i];
    }

     printf("%d %d %d %d %d %d %d %d %d %d\n min: %d, max: %d",
         n[0], n[1], n[2], n[3], n[4], n[5], n[6], n[7], n[8], n[9], min, max );
    return 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.