Writing the function strend()

Question

Due to being unable to ask a C programming language expert or advanced user in person, I'm posting this simple question about my C code. Today I was refreshing my C with exercise 5.4 of the C programming Language 2nd Edition by K&R, which is a very simple exercise:

Do you think that my way of handling pointers is 'very good' or 'world class'?

I know the example is short/simple but that question comes to my mind because I want to know the opinion from other knowledgeable people about the subject, if you want to critique my code please do so. I'm always trying to push my current C knowledge to the next level.

/*----------------------------------------------------*/
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
/*
  Exercise 5-4.
  Write the function strend(s,t) 
  which returns 1 if the string t occurs at the
  end of the string s, and zero otherwise.
*/
int strend(char *s, char *t);

int main(int argc, char *argv[])
{
  int rt; 

  if ( rt = strend("Unix Powered", "red"))
     printf("occurs\n");

 return EXIT_SUCCESS;
}

/*
  find if 't' occurs at the end of 's'
*/
int
strend(char *s, char *t)
{
   while (*s)
      s++;
   if ( strlen(s) < strlen(t))
      return 0;
   s -= strlen(t);
   for (; *t ; s++, t++) {
      if ( *s != *t) 
         return 0;
   }
 return 1;
} 
/*----------------------------------------------------*/

Answer 1

You should not change the function parameters like you are doing to 's'. If the function becomes a little bit more complex it then becomes very hard to read. Instead create some local variables with descriptive names; trust the compiler to optimize the code for you.

---

The function will for any string 't' return 0 since strlen(s) will always be 0 (as others already have pointed out). You move the pointer 's' to end of string. You may want to do the check before the while loop instead.

if ( strlen(s) < strlen(t))
{
   return 0;
}

while (*s)
{
   s++;
}

---

you also call strlen(t) twice, that seems unnecessary but I guess that is due to your unwillingness to have local variables.

---

arguments to the function should be declared const since it is a good indication for the user of your function that you are not going to change the contents of strings

---

printf("occurs\n"); can be replaced with puts("occurs"); (or fputs), puts has the advantage that it cannot be hacked by putting in a format specifier so avoid using printf when you don't have to.

---

always write compound statements instead of single statements after conditions/loops etc. A common mistake is when somebody later wants to add more statements is to forget adding {} - especially when the code is not your own.

if (cond)
  dofoo();

..

if (cond)
  dofoo();
  andthis();

having {} from the beginning makes it more clear

if (cond)
{
  dofoo();
}

---

you should also check the function arguments to see whether they are null or empty. you start out directly by assuming s points to something and increment it. at the very least have asserts to see if caller is following the contract of the function, asserts only trigger in debug mode.

assert( s != NULL );
assert( t != NULL );
assert( strlen(s) > 0 );
assert( strlen(t) > 0 );

it is a common defensive programming technique to put asserts at the beginning and at the end of a function to ensure that the functionality is as expected. first you check if the caller contract is fulfilled, then that the expected result is reached.

Answer 2

I would write the function like this:

#include <string.h>

/* Return 1 if string s1 ends with s2; return 0 otherwise. */
int strend(const char *s1, const char *s2)
{
    size_t l1 = strlen(s1);
    size_t l2 = strlen(s2);
    return l2 <= l1 && memcmp(s2, s1 + l1 - l2, l2) == 0;
}

If we know the length of the strings (as here), then memcmp is usually the quickest way to compare them. That's because memcmp doesn't have to check for the end of the string (unlike strcmp), and because standard library implementations of memcmp tend to be well optimized, using word-at-a-time comparison and other tricks if possible.

Answer 3

As @LokiAstari points out, your code does not perform as intended since strlen(s) always returns 0.

As strlen() is an "expensive" operation, you should avoid calling strlen(t) twice.

K&R C is a bit dated. With C99, you'll want to write strend(const char *s, const char *t). Furthermore, I would suggest that instead of returning 0 or 1, it would be more useful to return a pointer to the beginning of the location in s where t is found (or NULL if not found).

Get out of the habit of omitting optional braces. By doing so, you are taking the first step towards a future coding accident.