Populating a structure

Question

I'm solving K&R exercise 1-13:

Write a program to print a histogram of the lengths of words in its input. It is easy to draw the histogram with the bars horizontal; a vertical orientation is more challenging.

Although the exercise itself is pretty simple, I've decided to apply a more "professional" approach and encapsulate (is that a correct word?) histogram into a structure:

typedef struct {
    /* Legend and character to 'draw' histogram with */
    char **legend, chr;
    /* Histogram data */
    int *data;
    /* Max. data value, bin width, space between bins, max. bin length */
    int datamax, binwidth, binmargin, binlength;
    /* Data and legend array lengths */
    size_t datasize, legendsize;
} chart_t;

Because it's still a toy program, I decided to not overcomplicate algorithm so my histogram is limited to only integer data (it's hard to represent floating point values graphically in console anyway).

Overall algorithm works more or less correctly, so I ask to review only my chart creation procedure. The whole point of this was to learn how to use dynamic memory (allocate, free, etc.).

#define BINWIDTH_DEFAULT 3
#define BINMARGIN_DEFAULT 1
#define BINLENGTH_DEFAULT 14

/* ... */

chart_t *create_chart(int *data, size_t ds, char **lgnd, size_t ls, char chr) {
    chart_t *out = (chart_t *) malloc(sizeof(chart_t));

    if(out == NULL)
        return NULL;

    out->chr = chr;
    out->datasize = ds;
    out->legendsize = ls;

    out->binwidth = BINWIDTH_DEFAULT;
    out->binmargin = BINMARGIN_DEFAULT;
    out->binlength = BINLENGTH_DEFAULT;

    out->data = (int *) calloc(ds, sizeof(int));

    if(out->data == NULL) {
        free(out);
        return NULL;
    }

    memcpy(out->data, data, ds);

    int dmax = 0;
    for(int i = 0; i < ds; i++)
        if(data[i] > dmax)
            dmax = data[i];

    out->datamax = dmax;

    char **legend = (char **) calloc(ls, sizeof(char *));

    for(int i = 0; i < ls; i++) {
        size_t len = strlen(lgnd[i]);

        if(len == 0)
            continue;

        char *curstr = (char *) calloc(len+1, sizeof(char));

        if(curstr == NULL) {
            if(i > 1)
                for(int j = 0; j < i; j++)
                    free(legend[j]);

            free(legend);
            free(out);

            return NULL;
        }

        strncpy(curstr, lgnd[i], len+1);

        legend[i] = curstr;
    }

    return out;
}

Haven't I missed anything? Do I have any potential memory problems in my procedure?

Answer 1

• A case cursor == NULL does not free(out->data).

• A test i > 1 is strange, and buggy. If i happens to be 1, you are leaking memory legend[0] points to.

• Do not cast the result of malloc

• Calling calloc in char *curstr = (char *) calloc(len+1, sizeof(char)); is an overkill:

  • sizeof(char) is guaranteed to be 1, and
  • the contents of curstr is immediately overwritten by strncpy, so its zero initialization was a waste of time.

• The body of the loop spells out

      legend[i] = strdup(lgnd[i]);
  

  in a very obfuscated way.

• Given no context, it is very hard to see why do you need all that dynamic allocation at all.

Answer 2

One additional problem is that the memcpy is not copying all the data. It is copying ds bytes, not ds elements. So it should be

    memcpy(out->data, data, ds * sizeof(*data));

which also makes the zero initializing done by calloc for out->data a waste of time.

Answer 3

In addition to all the other great advice you got, there's one that stood out for me but wasn't mentioned.

You allow creating your structure only by dynamic memory allocation. So if you later wanted to embed that structure into another, you'd be forced to use a pointer. That's not a problem in and of itself, but I think that unless you must have a particular allocation scheme, it's not bad to separate the initialization from the allocation itself. So you'd have something like

void initialize_chart(chart_t *out, int *data, size_t ds, char **lgnd, size_t ls, char chr) {
  /* As you did before */
}

chart_t *create_chart(int *data, size_t ds, char **lgnd, size_t ls, char chr) {
    chart_t *out = malloc(sizeof(chart_t));
    /* Your verification here */
    initialize_chart(out, data, size_t, lgnd, ls, chr);
    return out;
}

Now you can easily embed chart_t into other structures by value (or create chart_t variables of automatic storage duration), if the need ever arises. The initialization is the same.

Answer 4

UPDATE: I've rewritten my code according to vnp's answer and separated initialization and creation as StoryTeller suggested (also I wonder whether it's possible to reference people in posts).

chart_t *initialize_chart(chart_t *chart, int *data, size_t ds, char **lgnd, size_t ls, char chr) {
    if(chart == NULL)
        return NULL;

    chart->chr = chr;
    chart->datasize = ds;
    chart->legendsize = ls;

    chart->binwidth = BINWIDTH_DEFAULT;
    chart->binmargin = BINMARGIN_DEFAULT;
    chart->binlength = BINLENGTH_DEFAULT;

    chart->data = malloc(sizeof(*data) * ds);

    if(chart->data == NULL) {
        free(chart);
        return NULL;
    }

    memcpy(chart->data, data, sizeof(*data) * ds);

    int dmax = 0;
    for(int i = 0; i < ds; i++)
        if(data[i] > dmax)
            dmax = data[i];

    chart->datamax = dmax;

    char **legend = calloc(ls, sizeof(char *));

    for(int i = 0; i < ls; i++) {
        size_t len = strlen(lgnd[i]);

        if(len == 0) {
            /* This situtation should be handled more sensibly, like, 
             * probably, converting i to string and using it as a legend
             * entry, but (as far as I know for now) there's no 
             * straightforward way to do it in C so I'll write it later.
             */
            continue;
        }

        legend[i] = strdup(lgnd[i]);

        if(legend[i] == NULL) {
            for(int j = 0; j < i; j++)
                free(legend[j]);

            free(legend);
            free(chart->data);
            free(chart);

            return NULL;
        }
    }

    return chart;
}

chart_t *create_chart(int *data, size_t ds, char **lgnd, size_t ls, char chr) {
    chart_t *chart = malloc(sizeof(chart_t));

    if(chart == NULL)
        return NULL;

    chart = initialize_chart(chart, data, ds, lgnd, ls, chr);

    return chart;
}

initialize_chart function returns chart_t because there can be errors during initialization and returning pointer (or NULL) may simplify checking whether function call went successfully in ifs and loops. It also operates on it's chart argument and does not create a temporary variable. I wonder if it's a good thing to do.

Concerning dynamic allocation I thought that it would be a good thing to deep copy the data into the chart in case programmer (if it was a library) modifies or corrupts their own copy. On the other hand, the charts are kinda supposed to be dynamic, so let's think it was a dumb architectural decision.

I also wonder if char **legend = calloc(ls, sizeof(char *)); is an overkill since allocated memory is rewritten (and even if we just keep skipping empty lines, we have legendsize to constraint our loops from printing nonsense from non-initialized memory).

I probably shouldn't be so concerned about this task, but I'm still learning and for me it's more of a "code design" lesson than programming one. Thank you everyone.