Custom Concatenate in C

Question

I'm trying to write my own concatenate program. What I'm doing is getting two strings as input from argv, creating a third empty character array that holds the length of argv[1] + argv[2], and then use two for loops to insert the characters from each argv string into the third string.

My first for loop seems to be working fine buy my second for loop isn't doing anything. Any ideas?

#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[])
{
  char *string1 = argv[1];
  char *string2 = argv[2];

  int string1Len = strnlen(string1, 50);
  int string2Len = strnlen(string2, 50);

  char string3[string1Len + string2Len + 1];

  for (int i = 0; i <= string1Len; i++)
  {
    string3[i] = string1[i];
  }

  for(int i = (string1Len + 1); i <= (string1Len + string2Len); i++)
  {
    string3[i] = string2[i];
  }

  string3[string1Len + string2Len + 1] = '\0';

  printf("%s %d %d\n", string3, string1Len, string2Len);

  return 0;
}

Answer 1

The index values in both loops are incorrect:

• you should stop the first loop when i == string1Len, hence the test should be:

   for (int i = 0; i < string1Len; i++)
  

• you should use add string1Len to the index into the destination string so bytes from the second string are appended to those of the first string:

  for (int i = 0; i < string2Len; i++) {
      string3[string1Len + i] = string2[i];
  }
  

• the index for the null terminator is string1Len + string2Len, adding 1 is incorrect as indexing is zero based in C:

   string3[string1Len + string2Len] = '\0';
  

• you should test the actual number of arguments provided to the program to avoid undefined behavior if some are missing.

Here is a modified version:

#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[]) {
    if (argc < 3) {
        fprintf(stderr, "missing arguments\n");
        return 1;
    }
    char *string1 = argv[1];
    char *string2 = argv[2];

    int string1Len = strnlen(string1, 50);
    int string2Len = strnlen(string2, 50);

    char string3[string1Len + string2Len + 1];

    for (int i = 0; i < string1Len; i++) {
        string3[i] = string1[i];
    }

    for (int i = 0; i < string2Len; i++) {
        string3[string1Len + i] = string2[i];
    }

    string3[string1Len + string2Len] = '\0';

    printf("%s %d %d\n", string3, string1Len, string2Len);

    return 0;
}

Answer 2

You can simplify (and optimize) it by using the memcpy function

int main(int argc, char **argv) {
    if (argc < 3) return 1;
    const char *string1 = argv[0];
    const char *string2 = argv[1];
    const size_t string1Len = strlen(string1);
    const size_t string2Len = strlen(string2);
    char *string3 = malloc((string1Len + string2Len + 1) * sizeof(*string3));
    memcpy(string3, string1, string1Len * sizeof(*string1));
    memcpy(string3 + string1Len, string2, (string2Len + 1) * sizeof(*string2));

    printf("%s %zu %zu", string3, string1Len, string2Len);
    free(string3);
    return 0;
}

And as the others said, pay attention to the nul terminator

Answer 3

The problem in your second loop is that i starts beyond the beginning of the second string. However, if all you are trying to do is to write you own custom (max 50 chars) concatenate program, all you need to do is to printf the arguments one after the other and printf can help limit:

#include <stdio.h>

int main(int argc, char *argv[])
{
  for (int i = 1; i < argc; ++i) printf("%.50s", argv[i]);
  printf("\n");
  return 0;
}

There is no need to copy in memory to a VLA and print.

If you need to create a function that concatenates, you better use malloc - as you can't safely return a VLA: (note, the following example will not limit to 50 chars):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char* concat(int argc, char* argv[]) {
  size_t totalsize = 1;
  for (int i = 0; i < argc; ++i) totalsize += strlen(argv[i]);

  char* ret = malloc(totalsize);
  if (!ret) exit(1);
  if (!argc) *ret = 0;

  char* dst = ret;
  for (int i = 0; i < argc; ++i) {
    char* src = argv[i];
    while (*dst++ = *src++); /* Copy also the \0 */
    --dst;
  }
  return ret;
}

int main(int argc, char *argv[])
{
  char* str = concat(argc - 1, argv + 1); /* Skip the program name */
  printf("%s\n", str);
  free(str);
  return 0;
}

Note that the above examples will concatenate any number of strings.

Answer 4

There are two main issues in your code. Your first for loop copies the nul terminator from string1; so, anything you then add to your string3 after that will simply be ignored by functions like printf, because they see that nul as marking the end of the string.

In your second for loop, you have the same problem and, more critically, the i index you use is not valid for string2, as you have added the length of string1 to it.

Also, note that arrays in C start at zero, so you shouldn't add the 1 to the position of the final nul terminator.

Here's the "quick fix" for your current code:

    for (int i = 0; i < string1Len; i++) { // Use "<" in place of "<=" or we copy the null terminator
        string3[i] = string1[i];
    }
    for (int i = 0; i < string2Len; i++) { // Start "i" at 0 for valid "string2" index ...
        string3[i + string1Len] = string2[i]; // ... and add "string1Len" for the destination index
    }
    string3[string1Len + string2Len] = '\0'; // Arrays start at ZERO, so don't add 1 for "nul" terminator position 

However, there are some other points and possible improvements. Note that the strnlen function returns a size_t type, so you would be better off using that for your indexes. Also, as you know that the i index at the end of the first loop will still be valid for the next character, you can re-use that in the second loop (so long as you have declared it outside the first loop), and you can use a second index for the source string.

Also, as pointed out by chqrlie, you really should check that you have sufficient source data in the argv array.

Here's a version of your program with those additional changes:

#include <stdio.h>
#include <string.h>

int main(int argc, char* argv[])
{
    if (argc < 3) {
        // Error-handling code
        return 1;
    }
    char* string1 = argv[1];
    char* string2 = argv[2];
    size_t string1Len = strnlen(string1, 50);
    size_t string2Len = strnlen(string2, 50);
    size_t i, j;
    char string3[string1Len + string2Len + 1];

    for (i = 0; i < string1Len; i++) {
        string3[i] = string1[i];
    }
    for (j = 0; j < string2Len; j++, i++) {
        string3[i] = string2[j];
    }
    string3[i] = '\0';
    printf("%s %zu %zu\n", string3, string1Len, string2Len); // "%zu" for "size_t"
    return 0;
}

Answer 5

Your second for loop "did nothing" because the first one worked up to the \0 character and included it in string3, so it's better to set the condition that the for loop works up to that character

#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[])
{
  char *string1 = argv[1];
  char *string2 = argv[2];

  int string1Len = strlen(string1);
  int string2Len = strlen(string2);

  int i;

  char string3[string1Len + string2Len +1];

  for (i = 0; string1[i]!='\0'; i++)
  {
    string3[i] = string1[i];
  }
  string3[i]=' ';   //with space
  ++i;
  for(int j = 0; string2[j]!='\0'; j++)
  {
    string3[i] = string2[j];
    i++;
  }

  string3[string1Len + string2Len + 1] = '\0';

  printf("%s %d %d\n", string3, string1Len, string2Len);

  return 0;
}