Skip to main content
added 33 characters in body
Source Link
Corbin
  • 10.6k
  • 2
  • 30
  • 51

Over all, this is good, but there's unfortunately some very major performance problems, and I have a few minor design issuessuggestions.


A string can be reversed quite trivially in linear time. Your algorithm is quadratic.

You can just do a single backwards loop over the input string instead of this convoluted nested loop.


Since mon_string_rev doesn't alter string it should be a pointer to constant character.


Instead of asserting that out isn't null, it might be better to simply return null instead. That allows the caller to handle the error instead of program termination (and when assertions are disabled, it avoids a rather nasty segfault...).


If you're using C99 or newer (and you should be), then declare variables in the tightest scope possible (i.e. declare i in the for loop).


return !1;... what? Just return 0, or, better yet, return EXIT_SUCCESS.


Any time you have an infinite loop terminated by a simple condition, just rewrite it to directly use that simple condition. Terminating 'infinite' loops are very hard to understand when you weren't the person who wrote them, so any time they can be avoided they should be.

Just to illustrate, your loop could instead be: for (; len >= 0; --len) { ... }.


It doesn't really apply here since instrumentation was clearly the main purpose, but in general it's a bad idea to have functions output anything unless their sole purpose is output. (Sorry if you already know this :).)


When it doesn't meaningful affect performance or usability, I like to have the caller manage memory. It's more flexible, and it's more explicit (no question of "wait... do I release this???").

Over all, this is good, but there's unfortunately some major performance and minor design issues.


A string can be reversed quite trivially in linear time. Your algorithm is quadratic.

You can just do a single backwards loop over the input string instead of this convoluted nested loop.


Since mon_string_rev doesn't alter string it should be a pointer to constant character.


Instead of asserting that out isn't null, it might be better to simply return null instead. That allows the caller to handle the error instead of program termination (and when assertions are disabled, it avoids a rather nasty segfault...).


If you're using C99 or newer (and you should be), then declare variables in the tightest scope possible (i.e. declare i in the for loop).


return !1;... what? Just return 0, or, better yet, return EXIT_SUCCESS.


Any time you have an infinite loop terminated by a simple condition, just rewrite it to directly use that simple condition. Terminating 'infinite' loops are very hard to understand when you weren't the person who wrote them, so any time they can be avoided they should be.

Just to illustrate, your loop could instead be: for (; len >= 0; --len) { ... }.


It doesn't really apply here since instrumentation was clearly the main purpose, but in general it's a bad idea to have functions output anything unless their sole purpose is output. (Sorry if you already know this :).)


When it doesn't meaningful affect performance or usability, I like to have the caller manage memory. It's more flexible, and it's more explicit (no question of "wait... do I release this???").

Over all, this is good, but there's unfortunately some very major performance problems, and I have a few minor design suggestions.


A string can be reversed quite trivially in linear time. Your algorithm is quadratic.

You can just do a single backwards loop over the input string instead of this convoluted nested loop.


Since mon_string_rev doesn't alter string it should be a pointer to constant character.


Instead of asserting that out isn't null, it might be better to simply return null instead. That allows the caller to handle the error instead of program termination (and when assertions are disabled, it avoids a rather nasty segfault...).


If you're using C99 or newer (and you should be), then declare variables in the tightest scope possible (i.e. declare i in the for loop).


return !1;... what? Just return 0, or, better yet, return EXIT_SUCCESS.


Any time you have an infinite loop terminated by a simple condition, just rewrite it to directly use that simple condition. Terminating 'infinite' loops are very hard to understand when you weren't the person who wrote them, so any time they can be avoided they should be.

Just to illustrate, your loop could instead be: for (; len >= 0; --len) { ... }.


It doesn't really apply here since instrumentation was clearly the main purpose, but in general it's a bad idea to have functions output anything unless their sole purpose is output. (Sorry if you already know this :).)


When it doesn't meaningful affect performance or usability, I like to have the caller manage memory. It's more flexible, and it's more explicit (no question of "wait... do I release this???").

Source Link
Corbin
  • 10.6k
  • 2
  • 30
  • 51

Over all, this is good, but there's unfortunately some major performance and minor design issues.


A string can be reversed quite trivially in linear time. Your algorithm is quadratic.

You can just do a single backwards loop over the input string instead of this convoluted nested loop.


Since mon_string_rev doesn't alter string it should be a pointer to constant character.


Instead of asserting that out isn't null, it might be better to simply return null instead. That allows the caller to handle the error instead of program termination (and when assertions are disabled, it avoids a rather nasty segfault...).


If you're using C99 or newer (and you should be), then declare variables in the tightest scope possible (i.e. declare i in the for loop).


return !1;... what? Just return 0, or, better yet, return EXIT_SUCCESS.


Any time you have an infinite loop terminated by a simple condition, just rewrite it to directly use that simple condition. Terminating 'infinite' loops are very hard to understand when you weren't the person who wrote them, so any time they can be avoided they should be.

Just to illustrate, your loop could instead be: for (; len >= 0; --len) { ... }.


It doesn't really apply here since instrumentation was clearly the main purpose, but in general it's a bad idea to have functions output anything unless their sole purpose is output. (Sorry if you already know this :).)


When it doesn't meaningful affect performance or usability, I like to have the caller manage memory. It's more flexible, and it's more explicit (no question of "wait... do I release this???").