1
\$\begingroup\$

I wrote a function that dynamically stores statistical data. I tried to clone the functionality of how I had originally written my solution in Bash. Is there perhaps something I could improve, or do better?

Bash Solution

#!/bin/bash

data_set=()
while IFS= read -r -p 'Enter statistical data (empty line to quit):' input; do
  [[ $input ]] || break
  data_set+=("$input")
done

C Solution

double* get_data(int* data_size)
{
    double input;
    char line[4096];
    double* data_set = NULL;
    double* new_data = NULL;
    int size = 0;

    puts("Enter data (end w/empty line): ");
    while(fgets(line, sizeof(line), stdin))
    {
        if(line[0] == '\n') break;

        new_data = realloc(data_set, (size+1) * sizeof(double));

        if(sscanf(line, "%lf", &input))
        {
            if(new_data == NULL)
            {
                free(data_set);
                puts("Error (re)allocating memory");
                exit(1);
            }
           data_set = new_data;
           data_set[size++] = input;
       }
    }

    *data_size = size;

    return data_set;
}
\$\endgroup\$

2 Answers 2

0
\$\begingroup\$

I find this API to be a poor match for C. You would be better off swapping the return value and the in-out parameter. By returning the number of items read, the return value could also serve as an error indicator (returns -1 on failure; consult errno for the reason). It would also be similar to the way scanf() returns the number of items assigned. That is better than hard-coding the error-handling policy of printing and exiting. (By the way, error messages should go to stderr, not stdout.)

/**
 * Prints the prompt to out, then reads numbers from in.  (No prompt
 * if out is NULL).  Afterwards, data will point to a newly allocated
 * array of doubles, and the number of elements in the array is returned.
 *
 * On memory allocation failure, returns -1 and errno is set to ENOMEM.
 */
int get_data(FILE *out, const char *prompt, FILE *in, double **data)

By taking additional parameters out, prompt, and in, the function gains a lot more flexibility for nearly no additional work. You can internationalize the prompt or read from a file.

You shouldn't interleave realloc(), sscanf(), and the error checking for realloc(). That is wrong for two reasons:

  • You allocate space for another element even if sscanf() read junk
  • Error checking for a function call should always immediately afterwards

The correct sequence should be sscanf(), realloc(), and the error checking for realloc().


Here is the revised C code:

/* Documentation here */
int get_data(FILE *out, const char *prompt, FILE *in, double **data) {
    char line[4096];
    int size = 0;
    *data = NULL;

    if (out) {
        fputs(prompt, out);
    }

    while (fgets(line, sizeof(line), in)) {
        if (line[0] == '\n') break;
        double input;
        if (!sscanf(line, "%lf", &input)) continue;

        double *new_data = realloc(*data, (size + 1) * sizeof(double));
        if (!new_data) {
            free(*data);
            return -1;
            /* errno should still be ENOMEM from realloc() */
        }
        *data = new_data;
        (*data)[size++] = input;
    }
    return size;
}

One final remark: the Bash version prints the prompt once per line. To replicate that behaviour…

int get_data(FILE *out, const char *prompt, FILE *in, double **data) {
    char line[4096];
    int size = 0;
    *data = NULL;

    while ( out && fputs(prompt, out),
            fgets(line, sizeof(line), in) ) {
        if (line[0] == '\n') break;
        /* etc */
    }
    return size;
}
\$\endgroup\$
1
  • \$\begingroup\$ You've definitely opened my head up for some more ideas to add. I really appreciate the detailed answer. \$\endgroup\$
    – user27606
    Commented Oct 23, 2013 at 13:17
1
\$\begingroup\$

From your bash script, I assume that you expect a proper (no junk) input. In that case, you can just do

while ((c = getchar()) != '\n') {
    ungetc(c, stdin);
    scanf("%lf%*c", &x);
    printf("x = %g\n", x);
}

Of course, you'll replace printf with memory (re)allocation.

If you stick to your solution, I think that

if (sscanf(line, "%lf", &input))
{
    new_data = realloc(data_set, (size+1) * sizeof(double));
    ...

makes a bit more sense. The way you're doing it now, you (re)allocate the memory even if the input is not a real number. This can lose you all your old data, if realloc has to move the block (so new_data != data_set) in the same step in which input is not a real number.

Edit: As requested in the comments, I'll elaborate why and how the original code could lose the data. So, I'm talking about this code:

while(fgets(line, sizeof(line), stdin))
{
    if(line[0] == '\n') break;

    new_data = realloc(data_set, (size+1) * sizeof(double));

    if(sscanf(line, "%lf", &input))
    {
        if(new_data == NULL)
        {
            free(data_set);
            puts("Error (re)allocating memory");
            exit(1);
        }
       data_set = new_data;
       data_set[size++] = input;
   }
}

There are two things to consider:

  1. The sole purpose of the line

    if(sscanf(line, "%lf", &input))
    

    is to make sure that line contains a real number (and to put it in input, as a number). If we were sure that line contains a double, we wouldn't use if.

  2. The line

    new_data = realloc(data_set, (size+1) * sizeof(double));
    

    tries to reallocate the memory pointed to by data_set so that it would fit size+1 doubles. However, this may not be possible if the memory right behind the block pointed to by data_set is occupied.

    In that case, another memory block of the required size will be allocated, the data will be copied there, and its address will be returned as the return value of realloc, which we save in new_data. The old memory is freed

So, what happens if both of these things happen, i.e., if we don't input a number and the occupation of the memory is such that realloc cannot simply expand the occupied memory?

First, we do realloc:

new_data = realloc(data_set, (size+1) * sizeof(double));

Now, our memory old memory (the one pointed to by data_set) is freed, a new memory is allocated and pointed to by new_data, and the data is copied to this new memory.

But, since we didn't input the number, this if will have a false condition:

if(sscanf(line, "%lf", &input))

So, we do not enter this block and, among other things, we don't do

data_set = new_data;

Hence, for the next step, our data_set points to some freed memory (where our data used to be), while the pointer new_data will be lost, either with the next new_data = realloc(...) command, or when we exit the function. In both cases, our data is unreachable, and any action on data_set will have unpredictable (most probably wrong) results.

It is not very probable that this will happen, but it might, and the solution is simply to reallocate the memory if and only if we're going to work with it. Hence, my suggestion above.

\$\endgroup\$
3
  • \$\begingroup\$ Hmm, could you explain a little more or demonstrate what you're saying with losing data? The main thing, I want to make sure I've done everything correctly. \$\endgroup\$
    – user27606
    Commented Oct 21, 2013 at 17:10
  • \$\begingroup\$ I've tried to elaborate it more by editing the answer. \$\endgroup\$ Commented Oct 21, 2013 at 17:33
  • \$\begingroup\$ Appreciate that so much! I understand now. \$\endgroup\$
    – user27606
    Commented Oct 21, 2013 at 23:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.