6
\$\begingroup\$

This program is designed to interleave two strings, which are input as command line arguments. I'd appreciate any feedback as I am new to C and self-taught.

Edit: Since this is an exercise in becoming more familiar with the intricacies of strings and memory allocation, the output should be stored before being printed.

#include <stdio.h>  // printf
#include <stdlib.h> // malloc, EXIT_FAILURE/SUCCESS
#include <string.h> // strlen

int main(int argc, char const *argv[])
{
  // Expecting: exe name, str_a, str_b
  if (argc != 3)
  {
    return EXIT_FAILURE;
  }

  size_t str_a_len = strlen(argv[1]);
  size_t str_b_len = strlen(argv[2]);
  if (str_a_len != str_b_len)
  {
    return EXIT_FAILURE;
  }

  // +1 for NULL byte
  size_t str_c_len = str_a_len + str_b_len + 1;
  char * str_c = malloc(str_c_len);

   // No free memory
  if (strlen(str_c) == 0)
  {
    return EXIT_FAILURE;
  }

  // Zip letters, AAA BBB => ABABAB
  for (int i = 0, n = str_c_len; i < n; ++i)
  {
    str_c[i] = (i % 2 == 0) ? argv[1][i / 2] : argv[2][i / 2];
  }

  printf("%s\n", str_c);

  return EXIT_SUCCESS;
}

I am compiling with:

gcc *.c -Wall -g -o zip.exe
\$\endgroup\$
  • \$\begingroup\$ You said in a comment: "this was mainly an exercise in string manipulation" Please share your real objective with us. You seem to want to interleave two strings and copy them to the output. For that, your solution doesn't need to explicitly allocate memory. In absence of prose, all we have is the observed effect of your program. Nothing about its implementation is sacred. If your goal is to write a function that puts the interleaved result into a dynamically allocated memory block - say so! But then we get to comment on the presumed usefullness of such an API as well :) \$\endgroup\$ – Reinstate Monica Feb 10 '17 at 15:59
  • \$\begingroup\$ Of course you are correct. Edited the question to be clearer. Thanks! \$\endgroup\$ – JWT Feb 10 '17 at 16:17
7
\$\begingroup\$

You allocate a memory block:

char * str_c = malloc(str_c_len);

and then pass it to the strlen():

if (strlen(str_c) == 0)
    ....

Apart from the str_c == NULL condition, described by @sanecz, you can not expect any meaningfull result from that test. It can even cause a crash!
That's because malloc() is not defined to initialize the allocated block of memory in any way. It can contain a zero byte at any position, which will cause strlen() to return any value less than the requested size of the block str_c_len; or it can contain no NUL byte at all, which will cause strlen() to scan past the allocated block and trigger an Undefined Behavior.

Additionally, you do not need the allocated block. If you want only to make an interleaved output, just do that: print characters in appropriate order.

int main(int argc, char const *argv[])
{
    // Expecting: exe name, str_a, str_b
    if (argc != 3)
    {
        return EXIT_FAILURE;
    }

    size_t str_a_len = strlen(argv[1]);
    size_t str_b_len = strlen(argv[2]);
    if (str_a_len != str_b_len)
    {
        return EXIT_FAILURE;
    }

    for (int i = 0; i < str_a_len; ++i)
    {
        putchar( argv[1][i] );
        putchar( argv[2][i] );
    }

    putchar('\n');

    return EXIT_SUCCESS;
}

EDIT

As for the comment 'this was mainly an exercise in string manipulation': if you want to improve your programming skills, and string manipulation among them, then I'd recommend to organize your program into separate functions. For example:

  • let main() fetch input data and pass it to some processing routine, then print the output;
  • let some MakeInterleavedString() validate input and prepare output buffer;
  • and let some InterleaveTwoStrings() do actual processing.

For example:

char *InterleaveTwoStrings(const char *l, const char *r, char *sum)
{
    // assume input strings l, r of equal length
    // and output buffer sum twice that long

    char *out = sum;      // the place where output goes
    while(*l != 0)        // input not exhausted yet
    {
        *out ++ = *l ++;  // copy one char and advance input
        *out ++ = *r ++;  // and output positions
    }
    *out = 0;             // terminate the output string

    return sum;           // return the filled buffer
}

char *MakeInterleavedString(const char *l, const char *r)
{
    // test if strings l, r are equal length,
    // prepare a buffer to accomodate l,r contents
    // interleave data into a buffer and return it;
    // return NULL on error

    int llen = strlen(l), rlen = strlen(r);
    if(llen == rlen)
    {
        if(char *sum = malloc(2*llen + 1))    // +1 for terminating NUL char
            return InterleaveTwoStrings(l, r, sum);
    }

    return NULL;   // on (llen != rlen) or malloc() failure
}

int main(int argc, char const *argv[])
{
    // Expecting: exe name, str_a, str_b
    if (argc == 3)
    {
        const char *str_a = argv[1];
        const char *str_b = argv[2];

        if (const char *str_c = MakeInterleavedString(str_a,str_b))
        {
            printf("%s\n", str_c);
            free(str_c);
            return EXIT_SUCCESS;
        }
    }
    return EXIT_FAILURE;
}

Please note the call to free() function – it's not that important in such simple program, as all the allocated memory goes back to the operating system on the program termination. However, in library functions we should always release temporary buffers as soon as they are no longer used. Otherwise we may forget to release them later, and we may cause so called 'memory leaks', when long running process consumes more and more memory for no reason.

\$\endgroup\$
  • \$\begingroup\$ Of course, the dirty memory from malloc into strlen is such an rookie error. Thanks for the feedback. I appreciate that I can print characters in the correct order, this was mainly an exercise in string manipulation but you raise a good point. \$\endgroup\$ – JWT Feb 10 '17 at 13:42
  • \$\begingroup\$ @JWT Please see expanded answer. \$\endgroup\$ – CiaPan Feb 10 '17 at 21:55
3
\$\begingroup\$

malloc returns NULL when no more memory is available

strlen with a NULL pointer will result an undefined behaviour. You should check if str_c is not NULL.

if (str == NULL)
{
  return EXIT_FAILURE;
}

Even if the malloc is successful, the length of the string will always be equal to zero and the program will exit.

\$\endgroup\$
  • \$\begingroup\$ The length of the newly allocated memory block treated as a string will always be zero if you used calloc() for allocation, which guarantees initialization of the block with zero bytes (ASCII NUL character). With malloc(), however, the allocated block is not guaranteed to be initialized, so strlen() may return anything – or even not return anything. \$\endgroup\$ – CiaPan Feb 10 '17 at 13:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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