I'm trying to reverse a string in C. The reverse function simply assigns the character at a given location (in a for loop) to a temp object. I cannot see any logic errors within the program, and the program compile successfully under gcc 4.7.2 with this command:
gcc -Wall -std=c99 reverse.c
To recreate the problem:
1.) Run the program and enter a string into your shell
2.) Once finished inputting, press enter/and or your EOF signal.
The problem is that neither the original string is printed, or the reversed string. This is also an exercise from K&R second edition, if you have completed this exercise, a different solution to mine would be appreciated.
I think the bug is caused by the absence of a null character, the famous printf requires a null terminated string to print input to cin. The getline function assigns a null character to the end of the array, surely the null character will be the first character in the string thereto ending the printf (and thus no character/literal is printed).
#include <stdio.h>
#define MAXLINE 1000
int geline(char s[], int lim);
void reverse(char line[], int length);
int main()
{
char s[MAXLINE];
char t[MAXLINE];
int k, len;
while ((len = getline(s, MAXLINE)) > 0) {
if (len > 1)
reverse(s, len);
}
printf("%s", s);
return 0;
}
void reverse (char input[], int length)
{
char temp[MAXLINE];
int j = length;
for (int i = 0; i < length; ++i, --j) {
temp[i] = input[i];
input[i] = input[j];
input[j] = temp;
}
}
int getline(char s[], int lim)
{
int c, i;
for (i=0; (c=getchar()) != EOF && c!='\n'; ++i)
s[i] = c;
if (c== '\n') {
s[i] = c;
++i;
}
s[i] = '\0';
return i;
}
(I did my compiling with -Wall -std=c99 -O3 -g, the -g to allow use of gdb)
Here are the things I noticed and some ways of addressing them. I've tried to hew pretty closely to the style you started with (I would have converted the array decls in the prototypes to pointers, for example, but that's not necessary).
Your getline prototype was missing the t.
int getline(char s[], int lim);
In main, you don't actually need k, t[MAXLINE], and your printf should probably be in the loop so you'll see each word as it's reversed. Note that printf picks up a \n, since the getline below converts both newline and EOF-terminated lines to the same thing (without newlines):
int main()
{
char s[MAXLINE];
int len;
while ((len = getline(s, MAXLINE)) > 0) {
if (len > 0)
reverse(s, len);
printf("%s\n", s);
}
return 0;
}
In above, the getline(s, MAXLINE) could have been getline(s, sizeof(s) / sizeof(*s) - 1) although again, be careful of fencepost errors (note the - 1).
The reverse function can be greatly improved without going over to the madness of xor to skip having a variable (although Daffra's example is interesting, especially in that it correctly stops in the middle). Instead, having the sense to just index up to the halfway point is a clear win. Between that and dropping reducing the temp array to just a temporary character, your general style is retained.
void reverse (char input[], int length)
{
int max = length - 1; /* keep the final NUL in place */
for (int i = 0; i <= max / 2; ++i) {
char ch = input[i];
input[i] = input[max - i];
input[max - i] = ch;
}
}
In the above gcc -O3 can do a serious workover on the code, so there's no real reason to worry that long division is going to be performed on every loop test, etc. For example, gdb reports that i itself gets optimized out automatically, which is pretty interesting. Write good, readable code first, have some faith in your compiler, optimize later.
And last, getline benefits from testing against lim (CRITICAL!) and and converting newlines into NULs.
int getline(char s[], int lim)
{
int i, c;
for (i=0; (i <= lim) && ((c=getchar()) != EOF) && (c != '\n'); ++i)
s[i] = c;
s[i] = '\0';
return i; /* return the index to the final NUL, same as length w/o it */
}
Setting MAXLINE to 10 temporarily shows that this version handles overlong lines fairly gracefully, splitting them into two separate ones without losing any of the characters.
Be careful with strings to very clearly decide whether you want to describe them in terms of length, or in terms of the index to the NUL at the end. This affects how you phrase your loops, limits, variable names, etc, and obviously confusing them is a classic source of fencepost errors.
Hope this helps.
There are two logic errors:
int j = length; should be int j = length - 1;temp[i] = input[i] ... input[j] = temp;
There are two approaches for that last error:
- Define
temp as a single char: char temp; ... temp = input[i]; input[i] = input[j]; input[j] = temp;
- Use the correct index in
temp: temp[i] = input[i]; input[i] = input[j]; input[j] = temp[i]
Try this code:
#include <stdio.h>
#define MAXLINE 1000
int geline(char s[], int lim);
void reverse(char line[], int length);
int main () {
char s[MAXLINE];
char t[MAXLINE];
int k, len;
while ((len = getline(s, MAXLINE)) > 0) {
if (len > 1)
reverse(s, len);
}
printf("%s", s);
return 0;
}
void reverse (char input[], int length) {
char temp;
int j = length - 1;
for (int i = 0; i < j; ++i, --j) {
temp = input[i];
input[i] = input[j];
input[j] = temp;
}
}
int getline (char s[], int lim) {
int c, i;
for (i=0; (c=getchar()) != EOF && c!='\n'; ++i)
s[i] = c;
if (c== '\n') {
s[i] = c;
++i;
}
s[i] = '\0';
return i;
}
int j = length - 1; // Thanks to @chux
for (int i = 0; i < j; ++i, --j) { // or <= length / 2
char temp = input[i];
input[i] = input[j];
input[j] = temp;
temp is not needed, and not entirely correctly used.
You are twice swapping the values, which restores the swap on the second half of the cycling. :)
Your prototype misses a 't' (geline). Hence maybe
ssize_t getline(char **lineptr, size_t *n, FILE *stream);
is taken?
you can use this fast function :
inline char * reverse(char *p)
{
char *save=p;
char *q = p;
while(q && *q) ++q;
for(--q; p < q; ++p, --q)
*p = *p ^ *q,
*q = *p ^ *q,
*p = *p ^ *q;
return save ;
}
Please have a look at this code:
#include <stdio.h>
#define MAXLINE 1000
int geline(char s[], int lim);
void reverse(char line[], int length);
int main()
{
char s[MAXLINE];
int len;
while ((len = geline(s, MAXLINE)) > 1) {
if (len > 1) {
reverse(s, len);
printf("%s", s);
}
}
return 0;
}
void reverse (char input[], int length)
{
char temp;
int j = length-1;
for (int i = 0; i < j; ++i, --j) {
temp = input[i];
input[i] = input[j];
input[j] = temp;
}
}
int geline(char s[], int lim)
{
int c, i;
for (i=0; (c=getchar()) != EOF && c!='\n'; ++i)
s[i] = c;
if (c== '\n') {
s[i] = c;
++i;
}
s[i] = '\0';
return i;
}
Only 2 changes needed here, and it will do the reverse fine.
Inside function reverse just do this
int j = --length;
Instead of this:
input[j] = temp; //you should use
input[j] = temp[i];
