current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I'm learning C and have made this very simple function to compare two strings. I would like to know how it can be improved:

int match(char* string1, char* string2)
{
    size_t i = 0;

    while (string1[i] != '\0' || string2[i] != '\0') {
        if (string1[i] != string2[i]) {
            return 0;
        }
        ++i;
    }

    return 1;
}

I figured it will only do 2 comparisons per iteration, and 3 in the last iteration. Also, I believe it won't go beyond the end of any array because it will return before iterating beyond the null terminator.

share|improve this question

2 Answers 2

up vote 3 down vote accepted

Your parameters should be const and your loop need only to check one string for the terminator. For example in the following loop, if *s1 is 0 the loop stops because of the first condition. If *s1 is not 0 but *s2 is 0, the loop stops because of the 2nd condition.

int match(const char *s1, const char *s2)
{
    while (*s1 && (*s1++ == *s2++)) {
         // nothing
    }
    return *s1 - *s2;
}
share|improve this answer
up vote 0 down vote

In your while loop, you check whether string1[i] or string2[i] are not null. If they are of unequal lengths, they will have their null terminators at different values of i; because at least one is non-null at each point, your while loop would then continue forever. To fix this, all you have to do is change || to &&, because you need to make sure that both of your characters are valid.

Another improvement that I would consider would be changing the return value to be zero if they are equal; this would match the built-in strcmp function. Then, you could return i as the place at which the two strings diverge, also matching strcmp. This provides the programmer with more useful information from one function call without a loss in value, because 0 is false and everything else is true.

share|improve this answer
    
Don't worry, the termination works fine. If they are unequal length, the if-comparison inside the while-loop will be triggered, and return 0. –  200_success♦ Sep 13 '13 at 5:33
    
Right, I didn't consider that. In any case, it would be more readable for a && operator. Also, if OP cares about performance, he should know that && is short-circuiting: It will only evaluate the right operand if the left operand is true, providing some (small) performance gain. –  acrucker Sep 13 '13 at 19:53
    
Once you change || to &&, it no longer works correctly. When one string begins with the other, it would report them as being equal. –  200_success♦ Sep 13 '13 at 21:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.