tags:

views:

224

answers:

4

Hi - I have a function that recursively makes some calculations on a set of numbers. I want to also pretty-print the calculation in each recursion call by passing the string from the previous calculation and concatenating it with the current operation. A sample output might look like this:

3
(3) + 2
((3) + 2) / 4
(((3) + 2) / 4) x 5
((((3) + 2) / 4) x 5) + 14
... and so on

So basically, the second call gets 3 and appends + 2 to it, the third call gets passed (3) + 2 , etc. My recursive function prototype looks like this:

void calc_rec(int input[], int length, char * previous_string);

I wrote a 2 helper functions to help me with the operation, but they implode when I test them:

/**********************************************************************
 * dynamically allocate and append new string to old string and return a pointer to it
 **********************************************************************/
 char * strapp(char * old, char * new)
 {
     // find the size of the string to allocate
     int len = sizeof(char) * (strlen(old) + strlen(new));

     // allocate a pointer to the new string
     char * out = (char*)malloc(len);

     // concat both strings and return
     sprintf(out, "%s%s", old, new);

     return out;
 }

/**********************************************************************
 * returns a pretty math representation of the calculation op
 **********************************************************************/
 char * mathop(char * old, char operand, int num)
 {
     char * output, *newout;
     char fstr[50]; // random guess.. couldn't think of a better way.
     sprintf(fstr, " %c %d", operand, num);
     output = strapp(old, fstr);
     newout = (char*)malloc( 2*sizeof(char)+sizeof(output) );
     sprintf(newout, "(%s)", output);
     free(output);
     return newout;  
 }


void test_mathop()
{
    int i, total = 10;
    char * first = "3";
    printf("in test_mathop\n");
    while (i < total)
    {
        first = mathop(first, "+", i);
        printf("%s\n", first);
        ++i;
    }
}

strapp() returns a pointer to newly appended strings (works), and mathop() is supposed to take the old calculation string ("(3)+2"), a char operand ('+', '-', etc) and an int, and return a pointer to the new string, for example "((3)+2)/3". Any idea where I'm messing things up? thanks.

+2  A: 

One immediate problem I can see is:

int len = sizeof(char) * (strlen(old) + strlen(new));

does not allocate space for the NULL char at the end. So you need one additional char allocated.

codaddict
The NULL char is covered by sizeof(char)
kenny
@kenny No, it is not. Test it for yourself.
Péter Török
@kenny: lets say old is "1" and new is "2". Both have a strlen of 1. Now of you want to concatenate them into a single string. It should accommodate '1', '2' and the '\0', to mark the end of the string. So we need one additional char to the sum of the string lengths.
codaddict
+3  A: 

You should allocate an extra byte for the terminating 0.

And you should use strlen instead of sizeof here:

newout = (char*)malloc( 2*sizeof(char)+sizeof(output) );

Sizeof returns the size of char* (which is something like 4 on an average system) instead of the length of the string pointed to by output.

Moreover, as @Agnel rightly pointed out, i is uninitialized, although that does not make your program crash, just execute the loop for a random number of times.

Also, I would recommend strstr strcat for concatenating your strings.

Péter Török
I thought strstr was to find a substring in another string
sa125
@sa125 Oops, you are right. I meant `strcat` of course.
Péter Török
Not only he has to allocate space fro the trailing \0, but he also has to put \0 there.
sharptooth
@sharptooth good point indeed, worth mentioning explicitly. Although `strcat` would handle it automatically.
Péter Török
+2  A: 

Try this, for starters:

char * append_strings(const char * old, const char * new)
{
    // find the size of the string to allocate
    size_t len = strlen(old) + strlen(new) + 1;

    // allocate a pointer to the new string
    char *out = malloc(len);

    // concat both strings and return
    sprintf(out, "%s%s", old, new);

    return out;
}

This is just your code, with a number of fixes:

  • The input strings are not changed, so they should be declared const.
  • Add one to the size required for the new buffer, to have room for the terminator.
  • String lengths are best stored in variables of type size_t.
  • Removed needless scaling by sizeof (char).
  • Don't cast the return of malloc().
  • Don't define functions whose name start with str, that is a reserved space. Pointed out by commenter, thanks!

For performance you could make use of the fact that you're calling strlen() on both inputs, and avoid using sprintf():

char * append_strings(const char * old, const char * new)
{
    // find the size of the string to allocate
    const size_t old_len = strlen(old), new_len = strlen(new);
    const size_t out_len = old_len + new_len + 1;

    // allocate a pointer to the new string
    char *out = malloc(out_len);

    // concat both strings and return
    memcpy(out, old, old_len);
    memcpy(out + old_len, new, new_len + 1);

    return out;
}

This should be a bit quicker, but I haven't benchmarked it. Notice how the final memcpy() includes the terminator from the new string, so there's no need to manually set it.

unwind
It's technically a bad idea to give functions name starting with "str".
Cirno de Bergerac
@sgm You are right, but IMO this should be a comment on the original question.
Péter Török
@sgm: You're absolutely right, I'll edit to include that as an improvement.
unwind
A: 

Thanks for all the replies, they were very helpful - especially the strcat suggestions and finding out I need to allocate space for the '\0' char. I ended up using realloc to "stretch" the string and append leading and trailing characters. I also eliminated the strapp (append_strings) function and got it all to work inside mathop. Here's how:

/**********************************************************************
 * returns a pretty math representation of the calculation op
 **********************************************************************/
 char * mathop(char * previous, char operand, float num)
 {
     char *output, *temp, calculation[50];
     size_t newlen;

     // copy the previous data into the temp string     
     temp = (char*)malloc( strlen(previous) + 1 );
     output = (char*)malloc(2);

     strcpy(temp, previous);
     strcpy(output, "(\0");

     // create the string portion to append to the output
     sprintf(calculation, " %c %.1f)\0", operand, num);

     // reallocate the output to append the additional string
     newlen = strlen(temp) + strlen(calculation) + 1;
     temp = realloc(temp, newlen);

     // append the new data to output
     strcat(temp, calculation);
     output = realloc(output, strlen(temp) + 2);
     strcat(output, temp);
     printf("%s\n", output);

     free(temp);
     return output;  
 }

thanks again!

sa125