-2

I'm having a problem and I can not figure out why. In main I create 2 arrays of strings. The function accepts 2 strings and creates a new array of the appropriate size according to the following requirement: For example: Array 1: aviv , Array 2: 12 , The new array: a12v12i12v The new array must be exactly the size! And then send the new array to main and main to print it. I also print junk values. I checked the size of the new array and it is the right size. My code:

 char* CreateString(char* str1, char* str2)
    {
        int length1, length2, length3 = 0;
        int i,j, index_help = 0;
        char *str3 = NULL;
        length1 = strlen(str1);
        length2 = strlen(str2);
        for (i = 0; i < length1; i++) // Check the size of the new array

        {
            length3++;
            if (i == (length1 - 1))
            {
                break;
            }
            for (j = 0; j < length2; j++)
            {
                length3++;
            }
        }
        str3 = (char*)malloc(length3+1  * sizeof(char));
        if (str3 == NULL)
        {
            printf("There is not enough memory space\n");
            return 0;
        }
        for (i = 0; i < length1; i++) //Copying data

        {

            str3[index_help] = str1[i];
            if (i == (length1 - 1))
            {
                break;
            }
            for (j = 0; j < length2; j++) 
            {
                index_help++;
                str3[index_help] = str2[j];
            }
            index_help++;

        }
        return str3;
    }

    int main()
    {
        char *str1 = NULL, *str2 = NULL,*str4=NULL;
        int size1, size2,i;
        printf("enter the size of string number 1:\n");
        scanf("%d", &size1);
        printf("enter the size of string number2 :\n");
        scanf("%d", &size2);
        str1 = (char*)malloc((size1 + 1) * sizeof(char));
        if (str1 == NULL)
        {
            printf("There is not enough memory space\n");
            return 0;
        }
        str2 = (char*)malloc((size2 + 1) * sizeof(char));
        if (str2 == NULL)
        {
            printf("There is not enough memory space\n");
            return 0;
        }
        printf("Enter a value for the first string (the size is:%d):\n", size1);
        scanf("%s", str1);
        printf("Enter a value for the second string (the size is:%d):\n", size2);
        scanf("%s", str2);
        str4 = CreateString(str1, str2);
printf("%s",str4);
        printf("\n");
        return 0;
    }
aviv.L
  • 39
  • 9
  • 1
    `for (i = 0; i < length1; i++)` - The first `for` loop - What is the logic here? You do not need a loop to achieve what it is doing – Ed Heal Dec 25 '17 at 16:56
  • 1
    I don't see *any* arrays of strings. – Scott Hunter Dec 25 '17 at 16:56
  • 3
    junk chars at end of string: dead giveaway for forgetting to null terminate string. – Jean-François Fabre Dec 25 '17 at 16:56
  • Did you terminate `str3` before returning it? – iBug Dec 25 '17 at 16:57
  • `str3[index_help] = '\0';` just before returning it. That'll do – Jean-François Fabre Dec 25 '17 at 16:57
  • @EdHeal the first loop is to count how mach chars i need to the new arry (size) – aviv.L Dec 25 '17 at 17:04
  • Casting malloc is bad - See https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc – Ed Heal Dec 25 '17 at 17:04
  • @iBug What do you mean? – aviv.L Dec 25 '17 at 17:05
  • @aviv.L - Using a little maths will not require the loop. – Ed Heal Dec 25 '17 at 17:05
  • Perhaps try calculating the size of te resulting array with some kind of mathematical formula. Repeated addition... hmm... sounds vaguely familiar. – n. m. could be an AI Dec 25 '17 at 17:06
  • @EdHeal You're right, it can be solved mathematically. But I wanted to solve the exercise with loops. – aviv.L Dec 25 '17 at 17:08
  • Avoid loops. Anyway what is with this nonsense `if (i == (length1 - 1))` ? That could be put in the for condition – Ed Heal Dec 25 '17 at 17:10
  • 1
    Your malloc call is just wrong. 1. Do not cast it. 2. Use the proper arithmetic: `length3+1 * sizeof(char)` is not the same as `(length3+1) * sizeof(char)`. In this case you are lucky, as `sizeof char` is 1. But let's assume you are dealing with `int`s, then it just yields the wrong number of bytes. – Pablo Dec 25 '17 at 17:14
  • @Ed Heal Casting malloc is bad only for low-qualified programmers. Please do not provide references to very bad answers. – Vlad from Moscow Dec 25 '17 at 17:44
  • @Pablo You are mistaken. (length3+1) * sizeof(char) is equivalent to length3+1 * sizeof(char) and in turn is equivalent to length3+1 – Vlad from Moscow Dec 25 '17 at 17:45
  • 1
    @Vlad in the case of `sizeof(char)` that's true, because `sizeof(char)` is 1. I said that. I also said, let's assume it were int. Then `(length3+1) * sizeof(int)` and `length3+1 * sizeof(int)` are **not** the same. That was my point – Pablo Dec 25 '17 at 18:07

1 Answers1

0

The function should be declared like

 char* CreateString( const char* str1, const char* str2);

because neither string str1 nor string str2 are changed in the function.

You have to append the result string with a terminating zero.

The length of the resulted string can be calculated simpler without using loops.

As the function strlen has the return type size_t then variables that accept the result of a function call should be also declared as having the type size_t instead of the type int.

The function can be implemented the following way as it is shown in the demonstrative program.

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

char * CreateString(const char *s1, const char *s2)
{
    size_t n1 = strlen(s1);
    size_t n2 = strlen(s2);

    size_t n3 = n1 == 0 ? 0 : n1 + (n1 - 1) * n2;

    char *s3 = ( char * )malloc(n3 + 1);

    if (s3)
    {
        if (n2 == 0)
        {
            strcpy(s3, s1);
        }
        else
        {
            char *p = s3;

            while (*s1)
            {
                *p++ = *s1++;
                if (*s1)
                {
                    strcpy(p, s2);
                    p += n2;
                }
            }

            *p = '\0';
        }
    }

    return s3;
}

int main( void )
{
    char *p = CreateString("aviv", "12");

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

    free(p);

    p = CreateString("", "12");

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

    free(p);

    p = CreateString("a", "12");

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

    free(p);

    p = CreateString("av", "12");

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

    free(p);

    p = CreateString("av", "");

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

    free(p);

    return 0;
}

The program output is

"a12v12i12v"
""
"a"
"a12v"
"av"
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335