Order line reversing (reversing line order)

Question

I'm a student learning to code in C. I had to make a program for class to reverse lines of input from the keyboard (not character wise, but reverse the order of the lines input).

The program is working to the requirements of the assignment already, but feel free to have a look at my code and help me understand where I can make improvements to it. I am looking to learn as much as possible about the language.

Assignment:

Program will allow user to type in several lines of a poem. User signals the end of their input with a single dot on a newline (and enter). After input, the lines will be printed out from last entered to first. No line can be greater than 1000 characters in length. No max number of lines. Lines cannot contain a single '.' dot/period as the only character on the line. No use of global variables.

---

   1 #include <stdio.h>
   2 #include <stdlib.h>
   3 #include <string.h>
   4 
   5 int main(){
   6 
   7   /* array variables */
   8   int MAXSIZE = 1000;
   9   char **array;
  10   char *string;
  11   char inputStr[MAXSIZE+1];
  12 
  13   /* declare for my while loop parameter*/
  14   inputStr[0] = 0;
  15 
  16   /* logic variables */
  17   int i = 0;
  18   int j = 0;
  19 
  20   /* allocate inital space for a line */
  21   array = malloc(sizeof(char*) * 1);
  22 
  23   printf("Enter the poem:\n");
  24 
  25   /* if the line is only a period, end loop */
  26   while(*inputStr != '.'){
  27 
  28     /* get input from user, max 1000 chars per line */
  29     fgets(inputStr,MAXSIZE,stdin);
  30     inputStr[strlen(inputStr)-1] = '\0';
  31 
  32     if(*inputStr != '.'){
  33 
  34       /* allocate space to store the string and copy from input */
  35       string = malloc ( sizeof(char) * (strlen(inputStr)+1) );
  36       strncpy ( string, inputStr, strlen(inputStr)+1 );
  37 
  38       /* allocate space in the array for the string and copy it in */
  39       array[i] = malloc((sizeof(char*))*(strlen(string))+1);
  40       strncpy(array[i],string,strlen(string)+1);
  41 
  42       /* free the string, add to line count, reallocate with new line count */
  43       free (string);
  44       i++;
  45       array = realloc(array, (i + 1) * sizeof(char*));
  46     }
  47   }
  48 
  49   /* print the array in reverse order using line count*/
  50   for( j = i-1 ; j >= 0; j-- ){
  51     printf("%s\n", array[j]);
  52   }
  53 
  54   /* free the array strings and then the array */
  55   while(i--){
  56     free(array[i]);
  57   }
  58   free(array);
  59 
  60   return(0);
  61 
  62 }

Answer 1

First I need to mention that I have been learning C coding for some time but would call myself an intermediate beginner at best. So take what I say with the right amount of scepticism.

int MAXSIZE 

To define constants like buffer sizes it is better to use const int which make your intention clearer.

Also you should be more consistent with the spaces around your arithmetic operations:

For example, strlen(inputStr) - 1 is clearer than strlen(inputStr)-1.

You should be constant with the way you use functions and how you intend their arguments:

f(arg1, arg2) instead of sometimes f (arg1,arg2) and sometimes f ( arg1, arg2 ).

In addition to these points, which will only improve the readability of your code, I am pretty sure that you could copy directly from input into the array like this:

/* allocate space in the array for the string and copy it in */
array[i] = malloc((sizeof(char*)) *(strlen(inputStr)) + 1);
strncpy(array[i], inputStr, strlen(inputStr) + 1);

This way you can delete the intermediate step where you save the input into a separate string and allocate (and free!) memory for it.

Answer 2

1. This is not about your code but your post: Don't paste the linenumbers, the inhibit copy&paste and don't add anything.

2. Starting every block with an empty line is odd, but at least you are consistent.

3. Avoid types as much as possible, refer to the respective pointer instead:

   array = malloc(sizeof *array);
   

   Can you find the (somewhat benign) error caused by not following that rule in your code?

4. sizeeof(char) is per definitionem 1. No exceptions.

5. Only use strncpy for its intended use, which is putting a string into a fixed-size buffer which must be zeroed. You should use strcpy.

6. You break on any line starting with a point '.'. Fix that.

7. You are allocating a buffer for the string twice, do the copy twice, and then free one of them. Why?

8. malloc and realloc can fail. You assume they always succeed.