Need to optimize the following split() function code

Question

Problem statement:

Wanted to have an optimized split function which will search for given character in string and split the string into 2 sub-strings, before and after the character.

Example :

s1 = strdup("XYZ@30");
s2 = split(s1,"@");
Should get following output.
s1 = "XYZ"    
s2 = "30"

I have written following split(), but could somebody help me optimize it.

char * split(char *str1, char *ch)
{
    int i=0;
    char *str2;
    if(!(str1 && ch))
        return NULL;
    else
    {
        str2 = strdup(str1);
        while('\0'==str2)
        {
            if(*str2==*ch)
            {
                i++;
                str1[i]='\0';
                return (++str2);//If the ch is last in str1, str2 can be NULL    
            }
            str2++;
        }
    }
    return NULL;
}

Answer 1

Don't use strdup; just have str2 point to the character after the split character.

Answer 2

Before optimization thoughts, first make sure that your code is correct.

In the spitting function, you do, after the strdup, which is not standard C, but your course assumes you have it.

while('\0'==str2)
{
  // some splitting logic
}

You will never enter the loop this way, since str2 is of type char*.

You have to change the condition into this:

while(*str2 != '\0')

which actually says, while we are not in the end of str2, go into the loop!

Why strdup is considered evil?

I suggest that you focus on why your code is wrong, rather copy-pasting a solution found on the internet.

Answer 3

The trick is that you first want to find the index of the split point, and replace it with \0, and point s2 to the next character. That's all you need to do.

char * split(char *str1, char *ch)
{
    int i=0;
    char *str2;
    if(!(str1 && ch))
        return NULL;
    else
    {
        while(str1[i] != *ch) i++; // Get i into position
        /* Make sure i+1 is still in bounds here */
        str2 = str1 + i + 1; // Point str2 to the next character
        str1[i] = '\0'; // Replace the delimiter with \0 to null-terminate s1
    }
    return NULL;
}

However you'd also want to do some error checking and make sure that, for example if \0 is the delimiter given that you aren't pointing s2 to out-of-bounds memory.

Answer 4

char *split(char *str, char ch){//call : s2 = split(s1, '@');
    char *p = strchr(str, ch);
    if(p){
        *p = '\0';
        return p+1;
    }
    return NULL;
}

Answer 5

• Your program doesn't do what you want it to do, so fix that before attempting to optimize anything.
• In order to make the program more efficient, you need to know the total size of the allocated string. You need the string length and then allocate string length + 1 for null termination.
• As it turns out, strdup is just a wrapper around the strlen, malloc and strcpy functions. At the same time, note that strdup is a 100% superfluous non-standard function and therefore shouldn't be used.
• Because of the above, replace strdup with strlen, malloc, memcpy (memcpy is slightly faster than strcpy). Manually add a null termination character at the end of the string.
• The split function should call the strstr function to detect the occurrence of the sub-string, if any.

• If the sub-string was detected, you need to allocate exactly

  [original string length] - [index of sub string] + [1 byte null termination]
  

  and then copy the sub-string into that allocated chunk of memory, by using memcpy().

• Leave the original string as it was, but null terminate it at the spot where you found the sub string. Alternatively you could allocate memory for this too and make a hard copy, but note that allocation/reallocation are the bottlenecks in this program (if there are any bottlenecks at all...).

• Handle the cases where the sub string is not found.

• You need no check if the parameters passed were NULL, to ensure that is the responsibility of the caller, not the function.