Array manipulation exercise

Question

While trying to learn more about arrays in C, I tried to write some code that did the following:

• Read a stream of numbers from the stdin and store them in an array

• Print the array in the order the numbers have been stored (i.e. print the original array)

• Print the array in reversed order

• Sort and print the array

• Given an array, search whether an entered value is present in an array. If it is present, print the index otherwise print that the value doesn't exist.

It will be great if somebody could review it and let me know how to improve it.

#include <stdio.h>
#include <stdlib.h>
static void printArray(int array[],int startIndex,int endIndex){
 if(startIndex < endIndex){
 while(startIndex <= endIndex){
 printf(" %i ",array[startIndex++]);
 }
 }else{
 while(startIndex >= endIndex){
 printf(" %i ",array[startIndex--]);
 }
 }
 printf("\n");
}
static int cmpfunc(const void * a,const void * b)
{
 if(*(int*)a > *(int*)b) return 1; else return -1;
}
static void sortedArray(int* originalArray){
 qsort((void*)originalArray,(sizeof(originalArray)/sizeof(originalArray[0])),sizeof(originalArray[0]),cmpfunc);
 return;
}
static int getIndex(int value,int array[],int size){
 int i;
 for(i = 0;i < size;i++){
 if(array[i] == value){
 return i;
 }
 }
 return -1;
}
static void identifyTheIndices(int *arbitraryArray,int size){
 char buf[10];
 printf("Enter the value to search for..enter q to exit\n");
 while(fgets(buf,sizeof buf,stdin) != NULL){
 if(buf[0] == 'q'){
 break;
 }
 char *end;
 int value = (int) strtol(buf,&end,0);
 if(end != buf){
 int currentIndex = getIndex(value,arbitraryArray,size);
 if(currentIndex > -1){
 printf("Found the entered value %i at index %i\n",value,currentIndex);
 }else{
 printf("Entered value %i doesn't exist\n",value);
 }
 }
 printf("Enter the value to search for..enter q to exit\n");
 }
}
int main(int argc,char **argv)
{
 int counter = 0;
 if(argc > 1){
 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++; 
 }
 int size = sizeof(originalArray)/sizeof(originalArray[0]);
 printf("Printing out the original array\n");
 printArray(originalArray,0,size - 1);
 printf("Printing out the array in reverse\n");
 printArray(originalArray,size - 1,0);
 printf("Sorting the array in ascending order\n");
 qsort((void*)originalArray,size,sizeof(originalArray[0]),cmpfunc);
 printf("Printing out the sorted array\n");
 printArray(originalArray,0,size-1);
 int arr[] = { 47, 71, 5, 58, 95, 22, 61, 0, 47 };
 identifyTheIndices(arr,sizeof(arr)/sizeof(arr[0]));
 }
 return 0;
}

Answer 1

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.

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

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 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.

• sc_ray: I am learning a lot from your reviews. I had a question, why would I be passing the array parameter as a const? I can probably look this up but is const essentially making the reference to the array immutable, so that the array cannot be modified by the function?
• vishram0709: Thanks @William Morris for pointing that out. I meant to say that taking care of error senarios was needed in the code.
• William Morris: Yes that is right. The function cannot modify the array values if the array is passed const. Neither can any function called by that function with the array as a parameter. That allows the compiler to optimise more and tells users of the function that their data will not be changed. It is good practice to add const to pointer parameters where possible.

Answer 2

@sc_ray pretty good effort if you are new to C programming.

A few points in your code which you can improve:

1. Taking array values from command-line is not good. Its better you use scanf and proper data-type. Below are logs when I pass a very long int value as cmd-line argument. It fails to store the value and gives garbage.

   ./a.out 7 999999999999 0 123
   Reading input value 7 into array
   Reading input value 2147483647 into array
   Reading input value 0 into array
   Reading input value 123 into array
   Printing out the original array
   7 2147483647 0 123
   .....
   

2. Hope this is a code pasting mistake. Remove this arr array not needed.

   int arr[] = { 47, 71, 5, 58, 95, 22, 61, 0, 47 };
   identifyTheIndices(arr,sizeof(arr)/sizeof(arr[0]));
   

3. The searching of elements in array can be improved using Binary Search Algo rather than sequential search.

Rest all looks good.

• sc_ray: Thanks. I will modify the array search to be binary search. It is definitely more computationally efficient.