5 ms·
Bug #1: char * rev = malloc(strlen(s)); ... It should be malloc( strlen(s) + 1 ) to include the NULL terminator. Nitpick #1: char tmp = *(rev+i); shou
by palish 16y ago
Bug #1:
char * rev = malloc(strlen(s));
... It should be malloc( strlen(s) + 1 ) to include the NULL terminator.
Nitpick #1:
char tmp = *(rev+i);
should be
char tmp( rev[i] );
Nitpick #2:
i<strlen(rev)/2 ;
"strlen(rev)" is evaluated every iteration of the loop.
Nitpick #3:
*(rev+i) = *(rev+strlen(rev)-i-1);
*(rev+strlen(rev)-i-1) = tmp;
Those two strlen(rev)'s are evaluated every iteration of the loop.
Why not write it like this?
for ( int i(0), iLen( (int)strlen(rev) );
i < (iLen / 2);
++i )
{
Swap( rev[i], rev[iLen - 1 - i] );
}
}
- cpeterso 16y ago1. `char tmp( rev[i] );` is valid C++, but not C. 2. Your for() loop is still dividing `iLen/2` every iteration. Presumably the compiler would hoist the division out of the loop, but the code would still add some cognitive friction to future readers of this function. :) Here's my attempt: void strrev(char* s) { char* a = s; char* z = s + strlen(s) - 1; while (a < z) { const char c = *a; *a = *z; *z = c; a++; z--; } }
- palish 16y agoSurely that's less clear than "Swap( rev[i], rev[iLen - 1 - i] );"? And yes, the compiler would optimize out "iLen / 2", computing it once rather than every iteration.
- watmough 16y agoThis is the one I like. :D I have interviewed people, but I gave up asking coding questions, since no one could ever do them. And if you keep thumbs-downing people, you don't really get invited to interview any more people. Thanks again!
- watmough 16y agoBug #1, Haha. I deliberately didn't do any polishing. I just C-P the first thing that compiled without warning and ran the embedded test case. Not so sure about nitpick #1. I'll need to pull out K&R... nitpick #2, sure. nitpick #3, is that really what idiomatic C looks like? Yikes.