String copy function not copying string properly. What's wrong with my code?

Question

I'm trying to write a function that removes whitesapces from a string and convert it to lower case.

My code below doesn't return anything. Why?

char *removeSpace(char *st);

int main(void){
    char *x = "HeLLO WOrld ";

    x = removeSpace(x);
    printf("output: %s\n", x);

}

char *removeSpace(char *st)
{
    int c = 0;
    char *s = malloc(sizeof(strlen(st)+1));
    for (int x = 0; x < strlen(st); x++)
    {
        if (st[x] != ' ')
        {
            s[x] = tolower(st[x]);
        }
    }
    st= s;

    st= s;
    return st;
}

Answer 1

The malloc statement uses sizeof unnecessarily as mentioned in the comments. You also have an error in the assignment of characters to the new string:

s[x] = tolower(st[x]);

You use the same index to the new string s as the old string st. This isn't right as soon as you remove any spaces. So for example indexes 0 through 4 line up between the two strings as you copy hello but then you skip a space at index 5 and then you want to assign the w at st[6] to s[5]. This means you need a separate index to track where you are in the destination string. So you need something like this code, which cleans up malloc(), adds the missing header includes, and introduces a new index for the output string:

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

char *removeSpace(char *st);

int main(void){
    char *x = "HeLLO WOrld ";

    x = removeSpace(x);
    printf("output: %s\n", x);

}

char *removeSpace(char *st)
{
    size_t len = strlen(st);
    int newStrIdx = 0;
    char *s = malloc(len+1);
    for (int x = 0; x < len; x++)
    {
        if (st[x] != ' ')
        {
            s[newStrIdx++] = tolower(st[x]);
        }
    }
    s[newStrIdx] = '\0';

    return s;
}

Oh, and you forgot the null-terminate the output string, which I added at the end.

Answer 2

For starters if you want to create a copy of a string then the function declaration shall look like

char * removeSpace( const char *st);

that is the original string is not changed within the function.

And as you are passing to the function a string literal

char *x = "HeLLO WOrld ";

x = removeSpace(x);

then indeed it may not be changed within the function. Any attempt to change a string literal results in undefined behavior.

The expression used in the call of malloc

sizeof(strlen(st)+1)

is equivalent to the expression

sizeof( size_t )

due to the fact that the function strlen has the return type size_t.

So this expression does not yield the length of the source string.

Moreover there is no need to allocate a string with the size equal to the size of the source string because the destination string can have much less characters (due to removing spaces) than the source string.

The assignment in the if statement

    if (st[x] != ' ')
    {
        s[x] = tolower(st[x]);
    }

uses an invalid index in the expression s[x]. That is as a result the destination string will contain gaps with uninitialized characters.

Also the terminating zero character '\0' is not appended to the destination string

Take into account that the set of white space characters includes other characters as for example the tab character '\t' apart from the space character ' '.

The function can be defined the following way.

char * removeSpace( const char *st )
{
    size_t n = 0;

    for ( const char *src = st; *src; ++src )
    {
        if ( !isspace( ( unsigned char )*src ) ) ++src;
    }

    char *result = malloc( n + 1 );
    result[n] = '\0';        
    
    for ( char *dsn = result; *st; ++st )
    {
        if ( !isspace( ( unsigned char )*st ) )
        {
            *dsn++ = tolower( ( unsigned char )*st );
        }
    }

    return result;
}

And the function can be called like

char *st = "HeLLO WOrld ";

char *dsn = removeSpace( st );

puts( dsn );

free( dsn );

Here is a demonstrative program.

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

char * removeSpace( const char *st )
{
    size_t n = 0;

    for ( const char *src = st; *src; ++src )
    {
        if ( !isspace( ( unsigned char )*src ) ) ++src;
    }

    char *result = malloc( n + 1 );
    result[n] = '\0';        
    
    for ( char *dsn = result; *st; ++st )
    {
        if ( !isspace( ( unsigned char )*st ) )
        {
            *dsn++ = tolower( ( unsigned char )*st );
        }
    }

    return result;
}

int main(void) 
{
    char *st = "HeLLO WOrld ";

    char *dsn = removeSpace( st );

    puts( dsn );

    free( dsn );

    return 0;
}

Its output is

helloworld

Answer 3

char *s = malloc(sizeof(strlen(st)+1));

you have a couple of nested expressions, and you jumped exactly the wrong way in the comment thread (I guess it was 50:50).

• strlen(st) is the number of characters in the string st

• strlen(st)+1 is the correct number of characters to allocate for a copy

  ... looking good so far!

• sizeof(strlen(st)+1) is the size in bytes required to represent the type of that value. So if size_t is an 4-byte unsigned int, this sizeof expression is just 4.

  The value of the string length is thrown away at this point.

Now, you want to allocate enough bytes for the string, not enough bytes to save the string's length as a size_t value. Just remove the sizeof entirely.

Oh, and also - st = s doesn't do anything here. The variable st is local inside the function, and doesn't affect anything outside. Returning s is sufficient.