C string to array -uppercase character bug

Question

Given this string "red, blue, green" create an array that contains these colors as its elements. The code I've written below works but when I change the first letter of the colors to uppercase I get the output- Red, Blu\301-!Wree\316. How can I make this code more dynamic to work with words that start with uppercase as well? Thank you.

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


int findLength(char string[]){
    int l =0;

    for(l = 0; string[l]!='\0'; l++){

    }
    return l;
};


char *stringToArray(char string[]){
    int i = 0;
    int j = 0;
    char c = ',';
    int n = 0;
    int l = findLength(string);

    char *str = (char *)malloc(l * sizeof(char));

    while(string[i] != l){
        if(string[i] == c || string[i] != '\0'){
            for(n = j; n < i; n++){
                str[j++] += string[n];
            }

        }
        i++;

    }
    printf("%s\n", str);
    str = '\0';
    return str;
}


int main(int argc, const char * argv[]) {
    char *string = "red, blue, green";
    //char *string = "Red, Blue, Green"; 
    char *str = stringToArray(string);
    free(str);

    return 0;

}

Answer 1

The strange behaviour doesn't have anything to do with whether your strings have upparcase letters or not. Your termination condition for the loop in stringToArray is wrong:

    int l = findLength(string);

    while (string[i] != l) ...

The condition should be

    while (i < l) ...

or, as you have already used in findLength:

    while (string[i] != '\0') ...

Because the condition is wrong – l is 16 in your case and none of the letters have an ASCII value of 16 – you go beyond the valid bounds of the string, which leads to undefined behaviour.

At the moment, you just copy the old string to the new one, albeit in a very strange fashion. Your inner loop makes use of three variables, of which it increments two. That's very confusing. It probably also doesn't do what you think, because the condition:

    if (string[i] == c || string[i] != '\0') ..

is true for all letters of the string, provided that the opuer loop should consider only valid characters up to, but not including the end of the string.

Finally, if you want to copy the string, you should allocate süpace for the terminating character:

    char *str = malloc(l + 1);

When you want to append the final null character:

    str = '\0';

You actually set the while allocated string to null, which leads to a memory leak. (The free in main doesn't produce an error, because free can legally take ´NULL` as argument.) Instead, use:

    str[l] = '\0';

With these fixes, you now have a program that copies the original string. The (POSIX) library function strdup does this more effectively. If you want to return an array of strings, you must reflect your function to return a pointer to pointers of chars.

Below is a possible implementation of that behaviour. (It uses the approach to allocate memory for everything on the heap. If you always expect three short strings that may not be the best solution.)

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

    char **stringToArray(const char *str)
    {
        char **res;
        const char *p = str;
        int n = 0;

        while (p) {
            p = strchr(p, ',');
            if (p) p++;
            n++;
        }

        res = malloc((n + 1) * sizeof(*res));

        p = str;
        n = 0;
        while (p) {
            const char *begin;
            size_t len;

            while (*p == ' ') p++;
            begin = p;

            p = strchr(p, ',');
            if (p) {
                len = p - begin;
                p++;
            } else {
                len = strlen(begin);
            }

            res[n] = malloc(len + 1);
            memcpy(res[n], begin, len);
            res[n][len] = '\0';

            n++;
        }

        res[n] = NULL;
        return res;
    }


    int main(int argc, const char * argv[])
    {
        char *str = "Vermilion, Ultramarine, Chartreuse"; 
        char **res = stringToArray(str);
        int i = 0;

        for (i = 0; res[i]; i++) {
            puts(res[i]);
            free(res[i]);
        }
        free(res);

        return 0;
    }

Answer 2

you have a few mistakes... I've corrected them for you:

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


int findLength(char string[]){
    int l = 1;

    for (int i = 0; string[i] != '\0'; i++){ 
        if (string[i] == ',')// to check the end of a color
            l++;
    }
    return l;
};


char **stringToArray(char string[]){//added a * for array of satrings
    int i = 0;
    int j = 0;
    char c = ',';
    int n = 0;
    int l = findLength(string);

    char **str = (char **)malloc(l * sizeof(char*)+l);
    char *pos = string;
    for (int i = 0; i < l-1; i++) //getting each color to the array
    {
        char *c =strchr(string, ',');
        int index = c - pos; 
        string[index] = 0;
        str[i] = _strdup(pos); //copying the color to the array
        pos = c + 1;
        string = string +1 +index; // next color
    }
    str[l - 1] = _strdup(pos); //copying last color

    for (int i = 0; i < l; i++) //printing the results
    {
        printf("%s\n",str[i]);
    }

    return str;
}


int main(int argc, const char * argv[]) {
    char string[] = "red,blue,green"; //deleted spaces
    char **str = stringToArray(string);
    getchar(); 
    free(str);

    return 0;

}

also added comments for you to understand.