current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite
1

This piece of code works fine. But I'm wondering if it can be done in a more efficient way. More specifically, this part (*(s1 + i)) if it possible to force it to sequence through entire array character by character via pointer, for example, *s1++.

My task to do this function compareStrings without index array []:

int  compareStrings(const char  *s1, const char  *s2)
{
    int i = 0, answer;
    //  i - to sequence through array of characters
    // pointer to character string1 and character string2
    while (*(s1 + i) == *s2 + i && *(s1 + i) != '\0'&& *(s2 + i) != '\0')
    {
        i++;
    }

    if ( *(s1 + i) < *(s2 + i) )
        answer = -1;               /* s1 < s2  */
    else if ( *(s1 + i) == *(s2 + i) )
            answer = 0;                 /* s1 == s2 */
        else
            answer = 1;                 /* s1 > s2  */

        return answer;

But I want to change it to s1++ and s2++ instead of *(s1 + i) and *(s2 + i). I've tried to implement this idea with pining an extra pointer to the beginning but I've failed.

int  compareStrings(const char  *s1, const char  *s2)
{
  int answer;
  char **i = s1, **j = s2;
  // i to sequence through array of characters
  while (*(i++) == *(j++) && *(i++) != '\0'&& *(j++) != '\0');

  if (*i < *j)
      answer = -1;               /* s1 < s2  */
  else if (*i == *j)
      answer = 0;                 /* s1 == s2 */
  else
      answer = 1;                 /* s1 > s2  */

  return answer;
}
share|improve this question
    
Your braces and indentation are off. Please check that your code is posted as intended. – 200_success♦ 8 hours ago
up vote 13 down vote accepted

You don't need pointers to character pointers at all:

int str_cmp(const char* s1, const char* s2)
{
    while (*s1 != 0 && *s2 != 0 && *s1 == *s2)
    {
        ++s1;
        ++s2;
    }

    if (*s1 == *s2)
    {
        return 0;
    }

    return *s1 < *s2 ? -1 : 1;
}

Also, there is a bug in your second implementation: it returns 1 on compareStrings("hello", "helloo"), when the correct result is -1.

Hope that helps.

share|improve this answer
    
It would be more understandable if you write the test for end-of-string as "*s != '\0'". – jamesqf 1 hour ago
up vote 3 down vote

Remember when you learned that arrays are nothing but the pointer to the first element of the same array? You can do this the other way around too: *(s1 + 1) is equivalent to s1[1].

In the second example take a look at how you resolve which pointer. **i = s1 so will *(i++) be equal to *(s1 + 1)? Your debugger can tell you the details from here :)

share|improve this answer
    
It says "+ i 0x0018e25c {0x69726561 <Error reading characters of string.>} char * * " – Yellowfun 19 hours ago
    
About it I know '*(s1 + 1) is equivalent to s1[1]'. :) – Yellowfun 19 hours ago
4  
"Remember when you learned that arrays are nothing but the pointer to the first element of the same array?" If so, then you were taught incorrectly. :-) What you say here is true, but it's because arrays decay implicitly to a pointer to their first element. The two things are not equivalent as far as the language is concerned. – Cody Gray 18 hours ago
1  
Agreed, it is an oversimplification. – fer-rum 18 hours ago
    
Thank you. I thought pointer to array is a & address in the memory to that array[]. But we can reach to it from start to end through the first element and than incrementing with something like index. – Yellowfun 14 hours ago
up vote 3 down vote

There's another huge bug in your code which stems from a quirk of C (and C++):
Plain char can be either signed or unsigned.

Next, do you really want to normalize the return-values to one of -1, 0 and 1, or is negative, zero, positive enough? The latter is what the standard does... and it's less work.

int compareStrings(const char* s1, const char* s2)
{
    while (*s1 && *s1 == *s2) {
        ++s1;
        ++s2;
    }
    return (int)(unsigned char)*s1 - (int)(unsigned char)*s2; // not normalized
    return *s1 == *s2 ? 0 : (unsigned char)*s1 < (unsigned char)*s2 ? -1 : 1; // normalized
}

(The code assumes that an int is bigger than a char.)

share|improve this answer
    
I've tried to implement your version of this function and it doesn't do the job that I need to at all. @Deduplicator – Yellowfun 15 hours ago
2  
This wrong. If string s2 is shorter than s1 you read past the end of s2. – Tonny 15 hours ago
1  
@Tony: How? That would mean they are different and thus stops the loop! – Deduplicator 13 hours ago
    
@Yellowfun: You mean you have to have your return-value normalized, or what? – Deduplicator 13 hours ago
    
Now it's working fine! @Deduplicator – Yellowfun 13 hours ago

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.