I am trying to learn C and have written a simple program that accepts the string from user and prints it. Would you loke to suggest me anything on my practices? I need to learn it very well. so please help me improving myself.
Here goes my code:

//Dynamic Array Allocation
#include <stdio.h>   //this is a c code
#include <conio.h>   //for using getch()
#include <stdlib.h>  //for using malloc,realloc, and free

void createACopy(char * copyTo,char * copyFrom, int length) //creates copy
{
    for(int i = 0; i < length; i++)  //loof for 'length' times
    {
        copyTo[i] = copyFrom[i];
    }
}

void main()
{
    printf("Please enter a string\n");
    char inputChar; //a characted input by user
    int inputLength = 0;    //holds the length of characters input so far
    char * userInput;   //a pointer that points to the beginnning of the user input
    userInput = (char *)malloc(sizeof(char));   //dynamically assign a single character size memory to the pointer
    if (userInput == NULL)  //if malloc could not find sufficient memory
    {
       free (userInput);    //free the memory
       puts ("Error Allocating memory");    //print error message
       exit (1);    //exit the program
     }
    do{ //keep looping till the user hits 'Enter' key
        inputChar = getch();    //get the character keyed in by user in inputChar variable
        if(inputChar ==char(8)) //if user hits backspace
        {
            inputLength--;  //decrease the length of user input by 1
            continue;   //continue and look for next character
        }
        char * storeOldInputHere = (char *) malloc(inputLength+1);  //dynamically find a memory location of size 'inputLenght'
        if (storeOldInputHere == NULL)  //if malloc could not find sufficient memory
        {
           free (storeOldInputHere);
           puts ("Error Allocating memory for copying the old input");
           exit (1);
         }
        createACopy(storeOldInputHere,userInput,inputLength);   //store the old Input here because realloc might give us a different location altogether.
        userInput = (char *) realloc(userInput,inputLength+2);  //now after we got a new character, reallocate memory.
        if (userInput == NULL)  //if realloc could not find sufficient memory
        {
           free (userInput);
           puts ("Error Reallocating memory");
           exit (1);
         }
        createACopy(userInput, storeOldInputHere,inputLength);  //Copy back the original input string to the newly allocated space again.
        userInput[inputLength] = inputChar; //append the new character user inserted.
        free (storeOldInputHere);   //free the storeOldInputHere
        inputLength ++; //increment the length counter by 1
    }while(inputChar != char(13));  //keep looping untill user hits 'Enter' key
    userInput[inputLength] = '\0';  //append a null charater at the end of the string
    printf("\nyou entered %d characters",inputLength);
    printf("\nyou entered: %s\n",userInput);
    free(userInput);    //free the userInput
}

Thanks in Advance

void main()

main() ALWAYS returns an int, declaring it as void could result in undefined behavor, read this thread

lines 16 and 17. Most C compilers and C standard before C99 require all variable declarations at the very beginning of a function or other scoped block marked with { and }. To be compatible with C standards older than C99 you should declare them at the beginning of a block. This may be important if your instructor wants to compile your program and does not use the same compiler that you use.

memory allocations: instead of expanding the string buffer one byte at a time it is much more efficient to allocate the memory in larger blocks, allocating too much memory is ok because the incased efficiency offsets the wasted memory. Computers have so much RAM now days that it's not even worth the effort to allocate exact amounts of ram. Here is one way to do it

void foo()
{
   const int BLOCKSIZE = 16;
   char *string = NULL;
   int input;
   int current_size = 0;
   int index = 0;
   while( (input = getch()) != EOF )
   {
       // check if we need to increase the length of the string
       if( (index+1) >= current_size)
       {
           current_size += BLOCKSIZE;
           string = realloc(string,current_size);
       }
       string[index++] = input;
    }
}

Edited 4 Years Ago by Ancient Dragon

Comments
Helped a lot.

This (and similiar blocks) does not make sense:

> if (userInput == NULL) //if malloc could not find sufficient memory
> {
> free (userInput); //free the memory
> puts ("Error Allocating memory"); //print error message
> exit (1); //exit the program
> }

If a pointer returned by malloc is NULL, then nothing was allocated. If nothing was allocated, then there is nothing to free. Freeing a NULL pointer will cause the program to crash. What you need to do is to free the memory only if the pointer returned is not NULL, e.g.

if (userInput != NULL)
{
    free(userInput);
    userInput = NULL;
}

Edited 4 Years Ago by MonsieurPointer

Freeing a NULL pointer will cause the program to crash.

That's incorrect. Calling free() on a null pointer is guaranteed to be a no-op.

Comments
So when you say it is a no-op, do you want me to still use it here onwards or I shouldnt.?

In addition to what The Dragon said:

1) Never use conio.h and getch() if at all possible. They are non-standard and are only defined in a couple compilers. Use a standard input function. In your case I understand why you used it, but in general, avoid getch()

2) Line up your comments as much as possible:

if (userInput == NULL)                //if malloc could not find sufficient memory
{
    free (userInput);                 //free the memory
    puts ("Error Allocating memory"); //print error message
    exit (1);                         //exit the program
}

3) Make your comments useful:

free (storeOldInputHere); //free the storeOldInputHere
inputLength ++; //increment the length counter by 1

Both comments are worthless. Good comments explain why the statement was used, not what the statement does.

4) realloc() does not loose the contents of the memory so the createACopy function is not necessary. See this

5) using exit(1) from the mainline is not necessary. return(1) will suffice. exit() is used when you are deep inside functions and must exit when returning with and error code is not an option.

I am really glad to join this group. Its just one single day and I am learning so many new things. Thanks to you all.
@WaltP:
Point number 4: I was under impression that once a new memory is allocated to my code, that pointer would be pointing to some random data present at that location at that time.I did not knew that realloc already does the 'createACopy' task for me.

TO all:
One more thing. I am new here. I would be trying to answer questions by others. But please cover me if I am giving incorrect informatiom.

Thanks Again.

Point number 4: I was under impression that once a new memory is allocated to my code, that pointer would be pointing to some random data present at that location at that time.I did not knew that realloc already does the 'createACopy' task for me.

New memory, yes, reallocated memory, no. It really does help to understand the functions you are using. Read before using so you know what you're doing.

One more thing. I am new here. I would be trying to answer questions by others. But please cover me if I am giving incorrect informatiom.

Be sure you test before answering. This includes testing code and verifying your information before responding.

It would really help if the person asking the question does a little checking first, but that rarely happens.

Edited 4 Years Ago by WaltP

Comments
Sure Thing.
This article has been dead for over six months. Start a new discussion instead.