HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Reading unlimited input strings in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
inputreadingstringsunlimited

Problem

The idea is to write a function that reads string of unknown size from stdin, keep it in dynamically allocated array of unknown size and return a copy of that array.

Is this the right approach? Could I be more clear? Would reading the input char-by-char be better than getting bigger chunks at once?

#include 
#include 
#include 

char * read_string(void)
{
    int buff_size = 11; /* size of temporary buffer */
    char temp[11]; /* temporary input buffer */
    char *output; /* pointer to allocated memory */
    char *copy; /* copy of input string to be returned */
    char *iterator; /* pointer to beginning of an array, used when searching for \n in input string */

    output = malloc(10*sizeof(char)); /* allocates 10 bytes for first part of input */

    if(output == NULL)
        return NULL;

    size_t size = 10*sizeof(char); /* keeps track of actual size of output */
    //*output = '\0'; /* places NUL at the beginning of output, to make it empty string */
    while(fgets(temp, buff_size,stdin) != NULL) /* read  max 10 chars from input to temp */
    {
        strcat(output,temp); /* append read input to output, without \0 char */
        if(strlen(temp) != 10 || temp[9] == '\n') /* if buffer wasn't full or last char was \n-stop because it reached the end of input string */
            break;

        size += buff_size - 1; /* if didn' reach end of input increase size of output*/
        output = realloc(output,size);

        if(output == NULL)
        {
            return NULL;
        }
    }

    iterator = output; /* search for \n in output and replace it with \0 */

    while(*iterator != '\n')
        iterator++;

    *iterator='\0';

    copy=malloc(strlen(output) + 1); /* allocate memory for copy of output +1 for \0 char */
    if(copy == NULL)
        return NULL;

    strcpy(copy, output); /* strcpy output to copy */
    free(output);  /* free memory taken by output */

    return copy;
}

Solution

Here are a few comments on your function:

-
Firstly, read_line seems a more accurate name, as you read up until the next \n.

-
Reading 10 chars at a time is an odd. Unless that is part of the requirement I think you should allocate a bigger buffer and read as many chars as fit, extending the buffer each time round the loop by doubling its size (instead of adding 11).

-
As you have it, your fgets call overwrites the char after the end of the buffer with \0. You should pass the actual size of the buffer to fgets.

-
Your fgets/strcat/strlen sequence is inefficient. You should be reading directly into the target buffer rather than reading and then copying. And the strcat call has to traverse the whole length of the accumulated string to concatenate the new part.

char *p = buf;
size_t len = 0;

while (fgets(p, size - len, stdin) != NULL) {
    len += strlen(p);
    if (buf[len-1] == '\n') {
        buf[len-1] = '\0';
        break;
    }
    ...
}


-
Your reallocation call leaks memory if it fails to allocate - you need a temporary variable to hold the return value from realloc:

size *= 2;
if ((p = realloc(buf, size)) == NULL) {
    break; // and then return an error if `p == NULL` or `ferror`
}
buf = p;
p += len;


-
Your loop exits when fgets returns NULL, but you do not check for an error.

You must call ferror to be sure that an error did not occur.

-
And your looping at the end to find the \n seems wasteful when you have already located the end of line/string in the main loop.

-
From the task statement you posted, copying the dynamic string is not required.

Note that reading character at a time is not necessarily a slower solution. As you are doing it (with all that the copying and string traversal), I would expect a well written character-at-a-time function to be faster. I encourage you to try both and compare the execution speed (using a very big test file as input).

Code Snippets

char *p = buf;
size_t len = 0;

while (fgets(p, size - len, stdin) != NULL) {
    len += strlen(p);
    if (buf[len-1] == '\n') {
        buf[len-1] = '\0';
        break;
    }
    ...
}
size *= 2;
if ((p = realloc(buf, size)) == NULL) {
    break; // and then return an error if `p == NULL` or `ferror`
}
buf = p;
p += len;

Context

StackExchange Code Review Q#31095, answer score: 3

Revisions (0)

No revisions yet.