Skip to main content
Code Review

Return to Answer

deleted 7 characters in body
Source Link
LastSecondsToLive
LastSecondsToLive
  • 747
  • 3
  • 13

That's a fine small piece of software for a beginner. Here is my review:

const-modifier
Since you don't modify the contents of the char *s you can safely change your function signature to void invert(const char *s).

Too many strlen() calls
You execute strlen(s) every for-loop iteration. This is quite bad for the performance (especially for large strings) since strlen loops through your whole string each call. If your string is 100 characters long (beside the NULL-character) this ends up in 100*100 = 10000 iterations.
As a quick solution you could just create a variable length and store the length of your string once. From that point on you'll compare with length instead of strlen(x) and will get the same result (since, s doesn't change while execution).

Comparison unused n < strlen(s) remains unused in your first for-loop. I think you want to connect the two comparisons with &&:

for (i = strlen(s) - 1, n = 0; n < strlen(s) && i >= 0; i--, n++)

Why even reverse?
Since your function prints the reversed string, but doesn't return anything. You don't really need to reverse it really. You could just loop through the input string starting at the end (like you did in your first loop), printing all the characters. The extra array only makes sense, if you return a pointer to it for later use.

That's a fine small piece of software for a beginner. Here is my review:

const-modifier
Since you don't modify the contents of the char *s you can safely change your function signature to void invert(const char *s).

Too many strlen() calls
You execute strlen(s) every for-loop iteration. This is quite bad for the performance (especially for large strings) since strlen loops through your whole string each call. If your string is 100 characters long (beside the NULL-character) this ends up in 100*100 = 10000 iterations.
As a quick solution you could just create a variable length and store the length of your string once. From that point on you'll compare with length instead of strlen(x) and will get the same result (since, s doesn't change while execution).

Comparison unused n < strlen(s) remains unused in your first for-loop. I think you want to connect the two comparisons with &&:

for (i = strlen(s) - 1, n = 0; n < strlen(s) && i >= 0; i--, n++)

Why even reverse?
Since your function prints the reversed string, but doesn't return anything. You don't really need to reverse it really. You could just loop through the input string starting at the end (like you did in your first loop), printing all the characters. The extra array only makes sense, if you return a pointer to it for later use.

That's a fine small piece of software for a beginner. Here is my review:

const-modifier
Since you don't modify the contents of the char *s you can safely change your function signature to void invert(const char *s).

Too many strlen() calls
You execute strlen(s) every for-loop iteration. This is quite bad for the performance (especially for large strings) since strlen loops through your whole string each call. If your string is 100 characters long (beside the NULL-character) this ends up in 100*100 = 10000 iterations.
As a quick solution you could just create a variable length and store the length of your string once. From that point on you'll compare with length instead of strlen(x) and will get the same result (since, s doesn't change while execution).

Comparison unused n < strlen(s) remains unused in your first for-loop. I think you want to connect the two comparisons with &&:

for (i = strlen(s) - 1, n = 0; n < strlen(s) && i >= 0; i--, n++)

Why even reverse?
Since your function prints the reversed string, but doesn't return anything. You don't really need to reverse it. You could just loop through the input string starting at the end (like you did in your first loop), printing all the characters. The extra array only makes sense, if you return a pointer to it for later use.

Source Link
LastSecondsToLive
LastSecondsToLive
  • 747
  • 3
  • 13

That's a fine small piece of software for a beginner. Here is my review:

const-modifier
Since you don't modify the contents of the char *s you can safely change your function signature to void invert(const char *s).

Too many strlen() calls
You execute strlen(s) every for-loop iteration. This is quite bad for the performance (especially for large strings) since strlen loops through your whole string each call. If your string is 100 characters long (beside the NULL-character) this ends up in 100*100 = 10000 iterations.
As a quick solution you could just create a variable length and store the length of your string once. From that point on you'll compare with length instead of strlen(x) and will get the same result (since, s doesn't change while execution).

Comparison unused n < strlen(s) remains unused in your first for-loop. I think you want to connect the two comparisons with &&:

for (i = strlen(s) - 1, n = 0; n < strlen(s) && i >= 0; i--, n++)

Why even reverse?
Since your function prints the reversed string, but doesn't return anything. You don't really need to reverse it really. You could just loop through the input string starting at the end (like you did in your first loop), printing all the characters. The extra array only makes sense, if you return a pointer to it for later use.