tags:

views:

160

answers:

4

We have a function longest, which returns the longest substring that consists of letters. Example:

longest("112****hel 5454lllllo454")

would return: lllllo

However, when I run the program it seems to return lllllo454. Here is the function:

char *longest(char *s){
    char *pMax = NULL;
    int nMax = 0;
    char *p = NULL;
    int n = 0;
    int inside = 0; //flag
    while(*s!='\0'){
        char c = *s;
        if((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')){
            if(inside == 0){
                n = 1;
                p = s;
                inside = 1;
            }
            else
                n++;
            if(inside == 1){
                if(n > nMax){
                    nMax = n;
                    pMax = p;
                    inside = 0;
                }
            }
        }//end isLetter if
        s++;
    }
    return pMax;
}

There's something I'm not seeing here...what do you guys think?

+6  A: 

You are just returning a pointer to the first character in the longest substring. You don't actually add a string terminator after the end of the substring, so it continues to the end of the original string. You probably ought to copy the substring (only those characters in the sequence) to a new string and return a pointer to that.

 char* newStr = malloc(nMax+1);
 strncpy( newStr, pMax, nMax );
 *(newStr+nMax) = '\0';
 return newStr;
tvanfosson
Or else have "int *nMax" as an output parameter. If so, best to have null checks before assigning to *nMax - it's a common "I don't need that" convention.
Steve314
+3  A: 

You are calculating nMax but not doing anything with that information. In C, a char* points to the start of a string of characters, which is terminated by a NUL character. Since you are not modifying the buffer passed to your function, the returned pointer points to the first 'l' and continues to the end of the original string.

Greg Hewgill
A: 

You are returning the pointer to the first letter of the longest substring; you are not making a new string out of it. Thus when you print it out, it prints the substring up until the NULL terminator.

See the strncpy function: http://www.cplusplus.com/reference/clibrary/cstring/strncpy/

I82Much
A: 

This returns the largest substring that starts with a string. The part of the function that sets inside, as follows:

if((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')){

will only be executed if c is a letter. Since you want the largest substring that includes a letter, you need this to be:

if(c != ' '){

Then, inside that loop, have another variable, say containsLetter, that is only true if you encounter a letter before another space.

Jeff Kelley