2

I wrote a function to compare 2 strings, return int as compare result, and pass an additional int pointer as param to retrieve the max match lengh.

// compare 2 strings

#include <stdio.h>

/**
 * compare 2 string,
 * 
 * @param sa
 *  string 1
 * @param sb
 *  string 2
 * @param len
 *  a int pointer pass from outside to store match length,
 * 
 * return
 *  0 if equlas, <0 if (a < b), >0 if (a > b),
 */
static int strCompare (char *sa, char *sb, int *len) {
    for((*len)=0; *sa==*sb; sa++,sb++, (*len)++) {
        // handle equals case, prevent ++ for \0,
        if(!*sa)
            break;
        // printf("%c,%c\n", *sa, *sb);
    }
    return *sa - *sb;
}

int main(int argc, char *argv[]) {
    if(argc < 3) {
        printf("need 2 arguments.\n");
        return 0;
    }
    int matchLen = 0;
    int result = strCompare(argv[1], argv[2], &matchLen);
    printf("compare:\n\t%s\n\t%s\nresult: %d\nmatch length: %d\n", argv[1], argv[2],
        result, matchLen);
    return 0;
}

Question:

I want the loop be more brief, e.g. avoid the if inside for, but didn't found out by myself, can anyone help to write a brief version with the same function interface.

(Please don't use libc function, I do this to improve my code style ;)

S.S. Anne
  • 13,819
  • 7
  • 31
  • 62
user218867
  • 16,252
  • 12
  • 112
  • 156
  • 4
    `I do this to improve my code style` Putting the if condition in the for will not improve your code style, it will make the code more "hacky" less readable and harder to maintain. If you want to improve your code style, add braces `{` around the if, and use built-in functions – Ivaylo Strandjev Dec 18 '14 at 11:19
  • You can probably add the (inverse of the) if condition inside the second section of your for loop. –  Dec 18 '14 at 11:22
  • 1
    Since `*sa==*sb` inside the loop you only need to check one of them in the `if`. – Klas Lindbäck Dec 18 '14 at 11:23
  • @IvayloStrandjev I write this to improve my c programming ability, so didn't use the libc, and want to improve the use of loop to make code brief, but yet your suggestion is good in real project. – user218867 Dec 18 '14 at 11:26

3 Answers3

3

You might want to avoid the repeated reads and writes through the pointer while you are at it, and go for const-correctness:

static int strCompare (const char *sa, const char *sb, int *len) {
    int tlen = 0;
    while(*sa && *sa == *sb)
        ++tlen, ++sa, ++sb;
    *len = tlen;
    return *sa - *sb;
}

Or maybe better with restrict:

static int strCompare (const char *sa, const char *sb, int * restrict len) {
    *len = 0;
    while(*sa && *sa == *sb)
        ++*len, ++sa, ++sb;
    return *sa - *sb;
}

BTW: The only thing making the code more efficient in the first case is avoiding the repeated writes through len.
In the second, it's using restrict and thus reducing aliasing (which will also remove all but the last write).

Also, consider whether size_t would not be a better type for the length.

Deduplicator
  • 41,806
  • 6
  • 61
  • 104
1

In your code if condition is needed. Because you are checking the pointer. If you accessing the pointer that is not allocate that will give you a segmentation fault. So avoid this you have to do the if condition. Or else you can made that in the for loop. 

for((*len)=0; *sa==*sb && *sa!='\0' ; sa++,sb++, (*len)++);

So avoiding the segmentation fault you need the another condition for checking.

Karthikeyan.R.S
  • 3,921
  • 1
  • 17
  • 31
  • As a point of clarity, avoid using just the semicolon for an empty statement. At least insert an empty inline-comment, `/**/;`, though better use an empty block instead `{}`. – Deduplicator Dec 18 '14 at 11:43
1

Perhaps something like:

static int str_compare(const char *a, const char *b, size_t *len) {
    const char *p = a;

    for ( ; *p && *p == *b; ++p, ++b)
        ;

    *len = p - a;
    return *p - *b;
}

As Duplicator has mentioned use const for input strings.

Also size_t is widely used for sizes and counts, so likely better.

Alternative by tracking length:

static int str_compare(const char *a, const char *b, size_t *n) {
    for (*n = 0; a[*n] && a[*n] == b[*n]; ++*n)
        ;

    return a[*n] - b[*n];
}

Does not look too good with all the indirection on n, but still.

As a side note; you should return 1 (or something other then 0) on error (in main).

Community
  • 1
  • 1
Morpfh
  • 3,793
  • 14
  • 23
  • I checked this code again, the logic is correct, and better. but maybe could avoid use so many pointer operation as suggested in other answer. – user218867 Dec 18 '14 at 14:33
  • @EricWang: Good. Was wondering if I was missing something here ... ;P – Morpfh Dec 18 '14 at 14:45
  • About return value in main(), you mean I should return value like -1 if the input is not correct? I am not sure does invalid input means error ... so just put it also 0, – user218867 Dec 18 '14 at 14:46
  • @EricWang: As the author of the code you are the boss, but yes, I would return an error on missing argument(s). That is the norm. In a way it becomes (an extension) of calling a function that require a set of arguments with none, or wrong type. If someone use your program in a script they often / sometimes will validate the result by checking return status. Error numbers can be anything you want, and is usually documented in `man` pages etc. If you want to be system specific and adhere to system codes you can look at e.g. http://www.virtsync.com/c-error-codes-include-errno – Morpfh Dec 18 '14 at 14:59
  • @EricWang: I often find `grep` as a good example of an in the middle approach. Here (often) 0 == found, 1 == not found, 2 == some error. http://unixhelp.ed.ac.uk/CGI/man-cgi?grep (DIAGNOSTICS). This makes it very helpful when it comes to automated / scripted use. – Else: http://en.wikipedia.org/wiki/Exit_status , http://stackoverflow.com/questions/1101957/are-there-any-standard-exit-status-codes-in-linux etc. – Morpfh Dec 18 '14 at 15:06
  • Thanks a ton, very useful info to me. – user218867 Dec 18 '14 at 15:09