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.

Write a function strend(s, t) which returns 1 if the string t occurs at the end of s, and 0 otherwise.

Here is my solution:

unsigned int strend(char *source, char *pattern) {
    char *saver = pattern;

    while(*source)
        source++;

    while(*saver)
        saver++;

    while(saver >= pattern) 
        if(*saver-- != *source--)     
        return 0;

    return 1;
} 

There are 2 while loops that will run until source and saver will point to the end of the string that they are representing. You will notice the presence of the variable saver, at the end of the second while loop this variable will have the value pattern + X, where X represent the number of charachters in the string pattern.

By using the variable saver I know when to stop the third loop. When saver is less than pattern that means that every charachter in the string pattern was compared with the coresponding one in source. At each iteration of the loop I check if the current charachter that saver points to is equal to the coresponding charachter in source.

Here is my second version:

unsigned int strend(char *source, char *pattern) {
    char *saver = pattern + strlen(pattern);
    source += strlen(source);

    while(saver >= pattern)
        if(*source-- != *saver--)
            return 0;
    return 1;
}

There is a dependency between my second implementation of strend and strlen. I, also, have some problems with the naming of the variable saver. What would be a more suggestive name for its purpose?

The exercise can be found at page 121 in K&R second edition.

share|improve this question
2  
The "dependency" on strlen in your second example is generally a good thing. Its reuse of existing functionality lets you focus on new problems, rather than ones that are already solved. (But it's good to know how to roll your own, too!) –  Michael Urman Feb 2 at 14:25
    
I propose changing char* to const char* –  luiscubal Feb 2 at 17:48
add comment

2 Answers

up vote 8 down vote accepted

I guess you have the naming problem because the code use the variable for multiple purposes. First, it points to the end of the pattern, then it's used for the pointer of the current character under test. (See below.)

In the first snippet there two identical while loops:

while(*source)
    source++;

while(*saver)
    saver++;

It's a nice catch that you changed them to strlen(). If you don't want #include <string.h> you still could extract it to a function to remove some code duplication:

char *getLastChar(char *input) {
    while(*input)
        input++;
    return input;
}

Here is a refactored version of the first snippet:

#define DIFFERENT 0
#define EQUAL 1

unsigned int strend1(char *source, char *pattern) {
    char *patternEnd = getLastChar(pattern);
    char *sourceEnd = getLastChar(source);

    char *currentPatternChar = patternEnd;
    char *currentSourceChar = sourceEnd;
    while (currentSourceChar >= source && currentPatternChar >= pattern) {
        if (*currentSourceChar != *currentPatternChar) {
            return DIFFERENT;
        }

        currentPatternChar--;
        currentSourceChar--;
    }
    return EQUAL;
}

Some things to note:

  1. I've added another check to the while loop: currentSourceChar >= source. It handles those cases (fixes a bug) when the pattern is longer that the source string.

  2. I've used two constants for the return values instead of magic numbers. It helps readers and maintainers because it shows the intent of 0 and 1.

  3. The patternEnd and currentPatternChar variables are basically the same (as well as their source pair) but I think it improves readability a little bit. First, you have a pointer to the last char (patternEnd), then another variables is used for iteration (currentPatternChar). (A good compiler will optimize it for you.)

share|improve this answer
add comment

I see no point in avoiding the use of Standard Library functions. Learning to use them is part of becoming a proficient C programmer. Hence using strlen is good and reinventing strcmp or memcmp, as you have done, is bad. Making the return value unsigned is just extra typing/noise and achieves nothing. Here is a simplified version:

int strend1(const char *str, const char *pattern)
{
    size_t plen = strlen(pattern);
    size_t slen = strlen(str);
    return (plen <= slen) 
           && !memcmp(pattern, str + slen - plen, plen) ? 1 : 0;
}

Note that the parameters are const.

share|improve this answer
    
I'm practicing my terminology. Can strend be considered a wrapper for memcmp? –  Ionut Grt. Feb 3 at 11:51
1  
A wrapper would modify the interface in some way, maybe providing a default parameter (eg the length for memcmp) or making the function work with different types. As such I'd expect the wrapper to be able 'naturally' (ie without it being contrived) to take a name derived from the wrapped function. I don't think that is really true of strend and memcmp - you might rename it memcmp_end but that does not really reflect what it does because no 'end' is defined, except in terms of string parameters, which memcmp does not take. So no, I don't consider it a wrapper. –  William Morris Feb 3 at 16:45
add comment

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.