Return to Answer

Minor formatting and typo correction

Source Link

edited Jan 27, 2014 at 22:22

9.4k
19
43

The fault he pointed out is caused by not validatingvalidating the input values. The same error could occur isif the value was input using scanf - and you saw in one of your earlier questions that scanf has its own issues. To check the input values you can use

printArray would be more normal taking a const start point and a length.
```
  static void printArray(const int array[], size_t n);
```
and have another printReverseArray to print the reveredreversed array.
cmpfunc should return 0 if the items match.
sortedArray is unused. It is also wrong in that
```
  (sizeof(originalArray)/sizeof(originalArray[0])),
```
gives something meaningless when originalArray is a pointer. sizeof(originalArray) gives the size of the pointer, not the array that it points to.

Later on in main, you do it right, with
```
  int size = sizeof(originalArray)/sizeof(originalArray[0]);
```
because here originalArray is a real array, not a pointer. But you already had the array size (argc - 1), so this was unnecessary.
the array parameter to getIndex should be const. But the function is unreliable, as it fails for negative numbers. To make it work you need to separate the return value from the success/failure.
```
  static int getIndex(int value, int array[], int size, int *index);
```
add some spaces, eg after if, while, for, ';'; and , etc.
you use the variable names array, originalArray and arbitraryArray to identify essentially the same thing - an array. Don't use multiple names for equivalent things without good reason.

Your reading loop

  int counter = 0;
  ...
  int originalArray[argc-1];
  while(counter < (argc - 1)){
      int currentValue = atoi(argv[counter+1]);
      printf("Reading input value %i into array \n",currentValue);
      originalArray[counter] = currentValue;
      counter++;      
  }

would be neater as:

  --argc;
  int array[argc];
  for (int i = 0; i < argc; ++i) {
      int v = atoi(argv[i + 1]);
      printf("Reading input value %i into array \n", v);
      array[i] = v;
  }

shorter variable names are ok, and indeed preferable, where their scope is restricted.

would be neater as:

    --argc;
    int array[argc];
    for (int i = 0; i < argc; ++i) {
        int v = atoi(argv[i + 1]);
        printf("Reading input value %i into array \n", v);
        array[i] = v;
    }

shorter variable names are ok, and indeed preferable, where their scope is restricted.

The fault he pointed out is caused by not validating the input values. The same error could occur is the value was input using scanf - and you saw in one of your earlier questions that scanf has its own issues. To check the input values you can use

printArray would be more normal taking a const start point and a length.
```
 static void printArray(const int array[], size_t n);
```
and have another printReverseArray to print the revered array.
cmpfunc should return 0 if the items match.
sortedArray is unused. It is also wrong in that
```
  (sizeof(originalArray)/sizeof(originalArray[0])),
```
gives something meaningless when originalArray is a pointer. sizeof(originalArray) gives the size of the pointer, not the array that it points to.

Later on in main, you do it right, with
```
  int size = sizeof(originalArray)/sizeof(originalArray[0]);
```
because here originalArray is a real array, not a pointer. But you already had the array size (argc - 1), so this was unnecessary.
the array parameter to getIndex should be const. But the function is unreliable, as it fails for negative numbers. To make it work you need to separate the return value from the success/failure.
```
  static int getIndex(int value, int array[], int size, int *index);
```
add some spaces, eg after if, while, for, ';' and , etc.
you use the variable names array, originalArray and arbitraryArray to identify essentially the same thing - an array. Don't use multiple names for equivalent things without good reason.

Your reading loop

  int counter = 0;
  ...
  int originalArray[argc-1];
  while(counter < (argc - 1)){
      int currentValue = atoi(argv[counter+1]);
      printf("Reading input value %i into array \n",currentValue);
      originalArray[counter] = currentValue;
      counter++;      
  }

would be neater as:

    --argc;
    int array[argc];
    for (int i = 0; i < argc; ++i) {
        int v = atoi(argv[i + 1]);
        printf("Reading input value %i into array \n", v);
        array[i] = v;
    }

shorter variable names are ok, and indeed preferable, where their scope is restricted.

The fault he pointed out is caused by not validating the input values. The same error could occur if the value was input using scanf - and you saw in one of your earlier questions that scanf has its own issues. To check the input values you can use

printArray would be more normal taking a const start point and a length.
```
  static void printArray(const int array[], size_t n);
```
and have another printReverseArray to print the reversed array.
cmpfunc should return 0 if the items match.
sortedArray is unused. It is also wrong in that
```
  (sizeof(originalArray)/sizeof(originalArray[0])),
```
gives something meaningless when originalArray is a pointer. sizeof(originalArray) gives the size of the pointer, not the array that it points to.

Later on in main, you do it right, with
```
  int size = sizeof(originalArray)/sizeof(originalArray[0]);
```
because here originalArray is a real array, not a pointer. But you already had the array size (argc - 1), so this was unnecessary.
the array parameter to getIndex should be const. But the function is unreliable, as it fails for negative numbers. To make it work you need to separate the return value from the success/failure.
```
  static int getIndex(int value, int array[], int size, int *index);
```
add some spaces, eg after if, while, for, ; and , etc.
you use the variable names array, originalArray and arbitraryArray to identify essentially the same thing - an array. Don't use multiple names for equivalent things without good reason.

Your reading loop

  int counter = 0;
  ...
  int originalArray[argc-1];
  while(counter < (argc - 1)){
      int currentValue = atoi(argv[counter+1]);
      printf("Reading input value %i into array \n",currentValue);
      originalArray[counter] = currentValue;
      counter++;      
  }

would be neater as:

  --argc;
  int array[argc];
  for (int i = 0; i < argc; ++i) {
      int v = atoi(argv[i + 1]);
      printf("Reading input value %i into array \n", v);
      array[i] = v;
  }

shorter variable names are ok, and indeed preferable, where their scope is restricted.

Source Link

answered Jan 27, 2014 at 17:54

William Morris

9.4k
19
43

I disagree with @vishram0709 about taking values from the command line. There is nothing wrong with this. It is often useful to have the option of taking values from the command line; you can also prompt for input values if none are given on the command line.

errno = 0;
char *end;
long value = strtol(string, &end, 0);
if (errno == ERANGE) {
    perror(string);
    exit(EXIT_FAILURE);
}

The input values above are now long not int because strtol converts to long and detects ERANGE based upon the size of the maximum possible long. If you wanted to detect over-range int values you could use:

char *end;
long value = strtol(string, &end, 0);
if ((int) value != value) {
    fprintf(stderr, "%s: Result too large\n", string);
    exit(EXIT_FAILURE);
}

Some other observations:

printArray would be more normal taking a const start point and a length.
```
 static void printArray(const int array[], size_t n);
```
and have another printReverseArray to print the revered array.
cmpfunc should return 0 if the items match.
sortedArray is unused. It is also wrong in that
```
  (sizeof(originalArray)/sizeof(originalArray[0])),
```
gives something meaningless when originalArray is a pointer. sizeof(originalArray) gives the size of the pointer, not the array that it points to.

Later on in main, you do it right, with
```
  int size = sizeof(originalArray)/sizeof(originalArray[0]);
```
because here originalArray is a real array, not a pointer. But you already had the array size (argc - 1), so this was unnecessary.
the array parameter to getIndex should be const. But the function is unreliable, as it fails for negative numbers. To make it work you need to separate the return value from the success/failure.
```
  static int getIndex(int value, int array[], int size, int *index);
```
add some spaces, eg after if, while, for, ';' and , etc.
you use the variable names array, originalArray and arbitraryArray to identify essentially the same thing - an array. Don't use multiple names for equivalent things without good reason.

Your reading loop

  int counter = 0;
  ...
  int originalArray[argc-1];
  while(counter < (argc - 1)){
      int currentValue = atoi(argv[counter+1]);
      printf("Reading input value %i into array \n",currentValue);
      originalArray[counter] = currentValue;
      counter++;      
  }

would be neater as:

    --argc;
    int array[argc];
    for (int i = 0; i < argc; ++i) {
        int v = atoi(argv[i + 1]);
        printf("Reading input value %i into array \n", v);
        array[i] = v;
    }

shorter variable names are ok, and indeed preferable, where their scope is restricted.