Hacker News new | past | comments | ask | show | jobs | submit login

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] );
  }
  }



1. `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--;
      }
  }


This 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!


Surely 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.


Bug #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.




Consider applying for YC's Spring batch! Applications are open till Feb 11.

Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: