Why code shows that strings are the same when one string is greater than the other

Question

I wrote a function that replaces the function strcmp().
The cases are:

1) strings are the same
2) second string will come first in the dictionary.
3) first string will come first in the dictionary.

In theory:

'a' > 'b'

So 'a' is the first string to come in the dictionary, however, my code doesn't exactly view it like this, instead it treats it like it's case 1.

Here is my code:

int cmp(char fString[], char sString[])
{
    int flag = 0;
    int i = 0;

    for (i = 0; fString[i]; i++) {
        if (fString[i] == sString[i]) {
            flag = 0;
        } else
        if (fString[i] > sString[i]) {
            flag = 1;
        } else {
            flag = -1;
        }
    }
    return flag;
}

The conditions are:

if (cmp(fString, sString) == 0) {
    printf("Strings are the same.\n");
} else
if (cmp(fString, sString) > 0) {
    printf("First string will come first in the dictionary\n");
} else {
    printf("Second string will come first in the dictionary\n");
}

Where did I do wrong?

Possible duplicate of [How do I properly compare strings in C?](http://stackoverflow.com/questions/8004237/how-do-i-properly-compare-strings-in-c) — tofro, Jan 21 '17 at 17:06
You shouldn't iterate over anything beyond the first character when comparing `"aardvark"` and `"zanzibar"`. Also -- what happens when `fString` is longer than `sString`? In that case `sString[i]` will eventually be out of bounds. — John Coleman, Jan 21 '17 at 17:10

score 2 · Answer 1 · answered Jan 21 '17 at 17:33

Your assumption is false: 'a' > 'b' should instead read

'a' < 'b'

If you insist on writing a function that returns 1 if the first string argument should come before the second argument, you should at least implement the correct algorithm:

you must stop the iteration when you encounter characters that differ. You currently keep looping and overwrite flag with the value for the next character.
you must deal with the case where the first string is shorter than the second, you currently return 0, which is incorrect.

Here is a corrected version:

int cmp(char fString[], char sString[]) {
    int i = 0;

    for (i = 0; fString[i]; i++) {
        if (fString[i] == sString[i]) {
            continue;
        } else
        if (fString[i] > sString[i]) {
            return 1;
        } else {
            return -1;
        }
    }
    if (sString[i])
        return 1;  // fString is shorter, it should come first 
    else
        return 0;  // strings are the same
}

Note however that it is very confusing to use counter-intuitive conventions. A string comparison function should return < 0 is its first argument is conceptually less the the second and should come first in a list sorted by increasing order.

Note also that the strcmp() function compares the strings according to the order of the type unsigned char instead of char.

Mohsen_Fatemi · Accepted Answer · 2017-01-21T17:18:13.983

first you must try to handle this problem , the length of both Strings are equal ? if not you , must work with minimum

notice that your implemented cmp function only works when the strings are equal

here is the problem of your cmp function , you must break your for loop when you know one String is bigger or less than other string , but your code continues checking remaining characters :

for(i = 0; fString[i]; i++)
    if(fString[i] == sString[i])
        flag = 0;
    else if(fString[i] > sString[i]){
        flag = 1;
        break;
    }else{
        flag = -1;
        break;
    }

if i was you , first i assume the strings are equal , then i would try to change the flag if String is greater than or less than other string

int flag = 0 , i = 0;
for(i = 0; fString[i]; i++)
        if(fString[i] > sString[i]){
            flag = 1;
            break;
        }else{
            flag = -1;
            break;
        }

score 0 · Answer 3 · answered Jan 21 '17 at 17:07

Your algorithm is wrong, you should return the difference between the characters when it's not 0 instead. Because the flag is overwritten if the last characters are the same, something very easy to happen if you include a trailing '\n' in your strings.

Note that by last I mean, the last character of the shortest string is the same as that of the longer string at the given position.

One quick fix is to check for flag == 0 in the for loop condition, but you can simply check if the difference between the two characters is 0 and continue with the next character or return it if it's not 0.

Why code shows that strings are the same when one string is greater than the other

3 Answers3