9
\$\begingroup\$

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?

Toby Speight
88k14 gold badges104 silver badges325 bronze badges
asked Jan 22, 2018 at 5:12
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . \$\endgroup\$ Commented Jan 22, 2018 at 8:57

4 Answers 4

8
\$\begingroup\$

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

answered Jan 22, 2018 at 5:42
\$\endgroup\$
6
\$\begingroup\$

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.

answered Jan 22, 2018 at 5:58
\$\endgroup\$
2
  • \$\begingroup\$ Good catch. Missed that. \$\endgroup\$ Commented Jan 22, 2018 at 6:41
  • \$\begingroup\$ Oh, that's a dumb mistake. Should've noticed it myself. Thank you. \$\endgroup\$ Commented Jan 22, 2018 at 8:14
3
\$\begingroup\$

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.

answered Jan 22, 2018 at 8:21
\$\endgroup\$
1
\$\begingroup\$

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.

answered Jan 22, 2018 at 9:00
\$\endgroup\$
6
  • \$\begingroup\$ If you return a pointer to support error checking, then you should really be doing error checking effectively. A blind chart = initialize_chart(...) is going to leak in case of an error. \$\endgroup\$ Commented Jan 22, 2018 at 11:14
  • \$\begingroup\$ But in this case if initialize_chart returns NULL, create_chart also will return NULL and it's now a responsibility of create_chart caller to check whether result is NULL. Or you're talking about indicating errors properly like setting errno? \$\endgroup\$ Commented Jan 22, 2018 at 11:24
  • \$\begingroup\$ I'm talking about you mallocing something without freeing it \$\endgroup\$ Commented Jan 22, 2018 at 11:24
  • \$\begingroup\$ But in case of initialization error, there's free(chart) in initialize_chart function. But I guess it isn't a good thing either. Thanks. \$\endgroup\$ Commented Jan 22, 2018 at 11:30
  • \$\begingroup\$ And what if chart wasn't allcoated with malloc? I posted my answer to suggest you support other allocation schemes. If it was a pointer to an automatic variable, you'd be in trouble just freeing it. If a function didn't allocate something itself, it has not business freeing it blindly either in case of error. \$\endgroup\$ Commented Jan 22, 2018 at 11:32

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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