Split text using delimiter?

Question

I wanted to create a function that will split a string by a delimiter.. I know there's already a function that does this thing but I wanted to make on my own.. But it doesn't work as it should.

char** Engine::splitString(const char* text, char delimiter)
{

  char** splitted;  
  splitted = (char**)malloc(50 * sizeof(char*));
  for (int y = 0; y < 50; y++)
    splitted[y] = (char*)malloc((strlen(text) + 2) * sizeof(char));

  int delimiterPosition[50];
  int arrayLength = 0;
  int f = 0;
  int g = 0;
  for (int x = 0; x < strlen(text); x++)    
  {
    if (text[x] == delimiter)
    {
        delimiterPosition[f] = x;
        f++;
    }
  }

  for (int x = 0; x < 50; x++)
    if (delimiterPosition[x] > 0 )
        arrayLength++;


  while (g < arrayLength) {
     if (g == 0) {
        for (int y = 0; y < delimiterPosition[0]; y++)
        {
            splitted[g][y] = text[y];
        }
    }
    else if(g > 0)
    {
        for (int y = delimiterPosition[g - 1]; y < delimiterPosition[g] - delimiterPosition[g - 1]; y++)
        {
            splitted[g][y] = text[y];
        }
    }
    g++;
}
return splitted;
}

First of all, I declared a two dimensional char array -> splitted. This was the variable that I should store my results into. Then I allocated a memory for it.. I wanted to have 50 words maximum. After that I created integer array.. this served as a storage for delimiters' positions. I also defined some variables below it for my code. Then I looped through the text to see if there's any delimiter.. if yes, I wanted to store it's position to a certain position in array, starting off from 0. I looped through delimiterPosition's array to how many positions I have stored. Then I made a simple loop using while to take all the characters up to the delimiter's position and store them to splitted[g][y] .. g represents the whole word.. y represents the character in that word. If g was greater than zero, I tok the previous position of a delimiter and then substracted the current from the previous.. and that gave me the distance between the first delimiter and the next..

The main problem here is that the first word is written correctly, the second one is not working, but it has some weird characters behind it when I try to call it.. is the text somehow leaking? the second one isn't being stored at all?:

    char** strings = en.splitString("Hello;boy", ';');
    printf("%s", strings[1]);

First word:

Second:

Any solutions, guys ? :) Thank you for any comment.

Answer 1

This does not initialize the memory:

 int delimiterPosition[50];

So its content is potentially random (and its undefined to read from unless you initialize it first). So here:

 if (delimiterPosition[x] > 0 ) // Is potentially invalid if x >= f

Easily solved with:

 int delimiterPosition[50] = {0};

---

Potential for overflow here:

        delimiterPosition[f] = x;
        f++;

You don't validate that f remains in the correct range (less than 50). Another easy fix:

  size_t stringLen = strlen(text); // Don't need to recalculate this each time!
  for (int x = 0; f < 50 && x < stringLen; x++)    
  {
    if (text[x] == delimiter)
    {
        delimiterPosition[f] = x;
        f++;
    }
  }

---

Here is the problem you are complaining about:

---

    for (int y = 0; y < delimiterPosition[0]; y++)
    {
        splitted[g][y] = text[y];
    }

You copy the string.
But you don't add a terminator to the string. So when you try and print it you see all the extra characters on the end.

    for (int y = 0; y < delimiterPosition[0]; y++)
    {
        splitted[g][y] = text[y];
    }
    splitted[g][y] = '\0';  // Add string terminator.

---

For the second a subsequent string you have the null terminator problem. But you also have the issue that you are copying the string to not the beginning.

        // After the first string the value of y in an offset into text only
        // So when used with `splitted[g]` you are offset from the beginning
        // if the string.
        splitted[g][y] = text[y];

Also your test for the end of the string is wrong:

Remember you start at:

int y = delimiterPosition[g - 1]

So y is an offset into the string. So as you increase it it will always be an offset not a length.

// So this test is wrong (you are using a length not an offset.
y < delimiterPosition[g] - delimiterPosition[g - 1]

Lets fix both at the same time:

    int dstIndex = 0;
    for (int y = delimiterPosition[g - 1]; y < delimiterPosition[g]; y++, dstIndex++)
    {
        splitted[g][dstIndex] = text[y];
    }
    splitted[g][dstIndex] = '\0';