current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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 <stdlib.h>
#include <string.h>
#include <stdio.h>


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;
}
share|improve this question
2  
I edited your code to make it more readable. Also, note that sizeof(char) is 1 by definition, so multiplying by it is redundant. –  busy_wait Sep 11 '13 at 12:06

2 Answers 2

up vote 2 down vote accepted

It would be wise to check if the reallocation of the memory worked.

Also, this:

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;

Why making a copy of output, freeing output, and then returning copy? Why not just returning output?

Here is a chunk of code I wrote a few years ago, which I find a bit simpler than your code:

char * read_string(void) {
  char *big = NULL, *old_big;
  char s[11] = {0};
  int len = 0, old_len;

  do {
    old_len = len;
    old_big = big;
    scanf("%10[^\n]", s);
    if (!(big = realloc(big, (len += strlen(s)) + 1))) {
      free(old_big);
      fprintf(stderr, "Out of memory!\n");
      return NULL;
    }
    strcpy(big + old_len, s);
  } while (len - old_len == 10);
  return big;
}

This was just a learning example, which I have never really used. In practice, I'd suggest working with a larger s (char s[1025], %1024[^\n] in scanf, and len - old_len == 1024 in the condition of the do...while loop) to reduce the number of realloc calls which can be very slow if the user gets unlucky. This also answers your last question: char-by-char might get very slow and is in no way better than chunks.

share|improve this answer
    
This is also a learning example, copy is part of assignment. –  zubergu Sep 11 '13 at 13:00
    
@zubergu Are you sure it was meant to be inside the function? Maybe it was meant to be called like this: output = read_string(); copy = malloc(strlen(output)+1); strcpy(copy, output); free(output);. –  Vedran Šego Sep 11 '13 at 14:34
    
Btw, I've edited reporting of an error in my code. –  Vedran Šego Sep 11 '13 at 14:36
    
task: "Write a function that reads a string from the standard input and returns a copy of the string in dynamically allocated memory. The funcion may not impose any limit on the size of the string being read. " English isn't my native l. but think i got it correct. –  zubergu Sep 11 '13 at 14:38
    
ATM I'm really proud that you didn't find any serious flaws, that would prove me entirely wrong and my code to be gibberish. –  zubergu Sep 11 '13 at 14:44
up vote 3 down vote

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.