3
\$\begingroup\$

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; } /*----------------------------------------------------*/
\$\endgroup\$
2
  • 5
    \$\begingroup\$ Neither because its broken. You have undefined behavior for this test: strend("s1", "longer string than s"); \$\endgroup\$ Commented Mar 23, 2015 at 6:32
  • \$\begingroup\$ My comment above provides an example that breaks your code. It just simply means you need to test the length of the second string. If it is longer than the first return 0. \$\endgroup\$ Commented Mar 23, 2015 at 6:48

3 Answers 3

Reset to default
5
\$\begingroup\$

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.

\$\endgroup\$
0
7
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ +1 for safety and elegance: Even if the rug gets pulled out from beneath you and the contents of 's2' change post-strlen(), your function still checks at most 'l2' bytes. \$\endgroup\$ Commented Mar 23, 2015 at 16:26
  • \$\begingroup\$ Gareth I must tell that I like your solution very much, it shows me some things that I have not think until now, thanks. \$\endgroup\$ Commented Mar 24, 2015 at 1:33
  • \$\begingroup\$ Gareth taking into account the overhead of function calls did you think that memcmp is more efficient in terms of speed than the comparing loop I made? I was seeing the man page (freebsd9) it seems to me that memcmp compares byte by byte, tell me if I'm wrong. \$\endgroup\$ Commented Mar 24, 2015 at 2:30
  • \$\begingroup\$ @cshlkws: both strlen and memcmp may be expanded inline by modern optimizing compilers. \$\endgroup\$ Commented Mar 24, 2015 at 23:05
  • \$\begingroup\$ This solution is also more consistent: strend(s, s), strend(s, "") and strend("", "") should all return 1. \$\endgroup\$ Commented Mar 24, 2015 at 23:06
3
\$\begingroup\$

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.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.