2
\$\begingroup\$

Since the previous question I posted here was not found as interesting as I expected, I decided to post a new one. I made a function solely for the purpose of having fun. That is, a function that checks if two strings are similar with a given %.

int case_insensitive_chrcmp (int ch1, int ch2)
{
    return ch1 == ch2 || ch1 + ' ' == ch2 || ch1 - ' ' == ch2;
}

int strpcmp (char* str1, char* str2, unsigned setting)
{
    int szStr1 = strlen(str1);
    int szStr2 = strlen(str2);
    int szMax  = max(szStr1, szStr2);
    int szMin  = 0;
    int chars  = 0;

    if(setting > 100)
        setting = 100;

    if(szStr1 == szMax)
        szMin = szStr2;
    else szMin = szStr1;

    chars = szMax * setting / 100;

    int cur = 0;
    for(int i = 0; i < szMin; i++)
    {
        if(case_insensitive_chrcmp(str1[i], str2[i]))
            cur++;
        else cur = 0;

        if(cur >= chars)
            return 1;
    }

    return 0;
}

(you may need to provide your own definition for min and max since I think they are not supported by all compilers)


Example usage:

char* str1 = "Hello";
char* str2 = "Hey";
int percentage = 50%101; // or just 50 or w/e
printf("%s %i%% %s - %s\n", str1, percentage, str2, strpcmp(str1, str2, percentage) ? "true" : "false");

Output:

Hello 50% Hey - true


More info on what the function is supposed to do:

It calculates how many characters must match consecutively based on percentage in order to return true. The comparison between characters is also done in case insensitive manner.

More info on how the function is required to work:

Regarding safety, this function can do much more, check against null pointers, re-entrancy guarding, MT-safety and that shouldn't really damage the performance level. Speaking of performance, I am a performancecholic and the first thing I can see that is bothering me here is the two strlens - the size of the two strings can be obtained within a single loop. Otherwise there are redundant iterations.

\$\endgroup\$
  • 1
    \$\begingroup\$ You probably want to use edit distance instead. \$\endgroup\$ – Emily L. Jun 30 '18 at 12:53
2
\$\begingroup\$

Alright I have a hole in my schedule, so I will give it a review myself.


Let's start with the function case_insensitive_chrcmp

  1. The name mixes two standards for function name, the well-descriptive, but long snake_case and the less descriptive but shorter standard abbreviation naming. So it would be better if it is either:

    • case_insensitive_char_compare
    • chrcasecmp

Since the second function uses the second standard, I would choose chrcasecmp.

  1. Many people do not like one-liners as they actually require more space (or even ironically more lines) than if they were not a function at all. Also it is a little bit slower (needs to access the routine, push the arguments onto the stack, etc). A good C function should be at least two lines and not more than a person with the old 640x480 monitor can see.

  2. It can be optimized and shortened further using the bitwise XOR operator:

return ch1 == ch2 || (ch1 ^ ' ') == ch2;

Since ' ' translates to 32, which is the sixth bit set and others unset.

And the strpcmp function

  1. I already said that two strlens to find the length of two presumably identical strings is not very efficient as it needs to iterate over both the strings, increment and check against '\0 at every iteration while this could be done in a single loop. This is an optimized function to find lengths of two strings:

int str2len (char* str1, char* str2, size_t * szStr1, size_t * szStr2)
{
    /* Variables */
    char*   p1      = str1;
    char*   p2      = str2;
    size_t  len1    = 0;
    size_t  len2    = 0;

    /* Pre-checking */
    if(str1 == NULL || str2 == NULL || szStr1 == NULL || szStr2 == NULL)
        return(-1);

    /* Processing */
    while(str1[len1] || p2[len2])
    {
        if(str1[len1] != '\0')
            len1++;
        if(str2[len2] != '\0')
            len2++;
    }

    /* Assignment */
    *szStr1 = len1;
    *szStr2 = len2;

    return 0;
}
  1. int cur = 0; is declaration after expression, which turns the code into C89-incompatible. Also the name cur is not very descriptive. While it may mean "current" it is not clear current of what it is. Better name nMatch.

  2. chars = szMax * setting / 100; Does not define what happens if the result from the division is not a whole number. In most cases the floating point number will be omitted which may or may not be the desired behavior. I should consider using round(), floor() or ceil() on a double

  3. The function checks if a char from str1 with index x matches a char with index x in str2 which makes the code susceptible to this scenario: "Hello World" and "World" will always return false. The function should rather search whether a characters are CONTAINED consecutively into the second string instead.

\$\endgroup\$
  • \$\begingroup\$ It is well unlikely str2len is faster then strlen(str1), strlen(str2). Quality strlen() will operate on 4 or 8 bytes at a time vs. this test against each null character twice each loop. \$\endgroup\$ – chux Jul 3 '18 at 3:37
  • \$\begingroup\$ "In most cases the floating point number will be omitted " is unclear. It is integer math with szMax * setting / 100. In all cases, floating point number will be omitted as there is none in that code. \$\endgroup\$ – chux Jul 3 '18 at 3:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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