C Programming Reversing Strings

[GenKiller]

Hey everyone. I'm attempting to reverse two strings in a program but am having difficulty. It does successfully reverse the string, but there are random symbols before the reversed string when printing the output. I'm assuming this is caused by the '\0' character being reversed, but my attempts at removing the first character have not yielded a result. Here is my code:

Code: Select all

#include<stdio.h>
#include<string.h>
#include<ctype.h>
  
int main(void)
{ 
  int stringCompare(char*, char*);
  int prefixCompare(char*, char*);
  int suffixCompare(char*, char*);
  char* commonChars(char*, char*);
  
  char string1[20], string2[20];
  int condition;
    
  /* Get strings from user */
  printf("Please enter two strings seperated by a space\n\tStrings: ");
  scanf("%s", string1);
  getchar();
  scanf("%s", string2);

  if(stringCompare(string1,string2)){
    printf("Strings are Equal\n");
    return 0;
  }
  condition = prefixCompare(string1,string2);
  if(condition){
    printf("Prefix of %d\n", condition);
    return 0;
  } 
  
  condition = suffixCompare(string1,string2);
  if(condition){ 
    printf("Suffix of %d\n", condition);
    return 0;
  }
  
  printf("Characters in common: %s\n", commonChars(string1, string2));
  
  return 0;
}
  
int stringCompare(char *string1, char *string2)
{ 
  if(!strcmp(string1,string2)){
    return 1;
  }
  return 0;
}

int prefixCompare(char *string1, char *string2)
{ 
  int i, length, length2;
  
  length  = strlen(string1);
  length2 = strlen(string2);

  if(length < length2){
    length = length2;
  }

  for(i=0; i<=length; i++)
    if(strncmp(string1,string2,i))
      return i-1;
  return 0;
}

int suffixCompare(char *string1, char *string2)
{
  int i, length, length2;
  char rev1[20];
  char rev2[20];

  length  = strlen(string1);
  length2 = strlen(string2);

  for(i=0; i <= length; i++){
    sprintf(rev1,"%s%c", rev1,string1[length - i]);
  }

  for(i=-1; i <= length2; i++){
    sprintf(rev2,"%s%c", rev2,string2[length - i]);
  }

  printf("\nOriginal String 1: %s\n", string1);
  printf("Original String 2: %s\n", string2);
  printf("Reversed String 1: %s\n", rev1);
  printf("Reversed String 2: %s\n", rev2);

  return prefixCompare(rev1, rev2);
}

char* commonChars(char *string1, char *string2)
{
  char *pointer = strpbrk(string1, string2);

  return &(*pointer);
}

The problem is located in the suffixCompare() function, which reverses the string and then sends it to prefixCompare() for evaluation.

I've attempted to remove the first character of the reversed strings via the following code as stated above, without any success.

Code: Select all

  strcpy (rev1, rev1+1);

Sample Run:

Code: Select all

Please enter two strings seperated by a space
        Strings: goater boater
Original String 1: goater
Original String 2: boater
Reversed String 1: E@dôÿ¿¨óÿ¿ ðóÿ¿Ñóÿ¿retaog
Reversed String 2: ß@@retaob
Characters in common: oater

**DISCLAIMER** This is [obviously] a program for school, so nobody says that I am trying to push my assignment on to others. I have searched for the answer high and low for a long time now (multiple hours, multiple days) and have yet to find an answer, or anything that would make the lightbulb in my head go off to why this may be happening. I hope it is evident that I have tried to do this on my own, and have yet to come up with a way.

I appreciate any help!!

[mdowney]

I didnt have time to read it all but just guessing you might be going past the size of the array and getting random bits from memory, i can remember doing that a few times.

[GenKiller]

I didnt have time to read it all but just guessing you might be going past the size of the array and getting random bits from memory, i can remember doing that a few times.

Seems like a different problem than that, as I now took and did a strlen(rev1) and it returned '30' (10 past the 20 allocated). This would go into what you are saying, but when I did a printf("%c",rev[30]); it did not return the correct character, but whitespace.

Thanks for the response, I appreciate the help (and keep the ideas coming!)

[Angrybob]

you have a mistake in the second reversing bit, you use "length" where you should use "length2", but that whole bit with using sprintf seems like overkill, just use this:

Code: Select all

// length-1 everywhere so we don't include the null terminating byte
for(i=0; i <= length-1; i++){
    rev1[i] = string1[length - i - 1];
  }
  rev1[length] = 0; //add the terminating byte back on

  for(i=-1; i <= length2-1; i++){
    rev2[i] = string2[length2 - i - 1];
  }
  rev2[length2] = 0;

[Kihaji]

#include<stdio.h>
#include<string.h>
#include<ctype.h>

int main(void)
{
Prototypes go outside the main
int stringCompare(char*, char*);
int prefixCompare(char*, char*);
int suffixCompare(char*, char*);
char* commonChars(char*, char*);

char string1[20], string2[20];
int condition;

/* Get strings from user */
printf("Please enter two strings seperated by a space\n\tStrings: ");
this could be written as scanf("%s %s", string1, string2);
scanf("%s", string1);
getchar();
scanf("%s", string2);

why dont you use strcmp() from the C std libraries?
if(stringCompare(string1,string2)){
printf("Strings are Equal\n");
return 0;
}

What is this doing? If I read the function below correct it searches for the first common character in the same position and returns that position -1, which is not something you want to test for logic. Rethink this or comment so we can understand what you want
condition = prefixCompare(string1,string2);
if(condition){
printf("Prefix of %d\n", condition);
return 0;
}

Name your methods better, this does not compare, it actually reverses. Names should be descriptive.
condition = suffixCompare(string1,string2);
if(condition){
printf("Suffix of %d\n", condition);
return 0;
}

printf("Characters in common: %s\n", commonChars(string1, string2));

return 0;
}

Why exactly did you need to make a wrapper method that adds nothing to the method it wraps?
int stringCompare(char *string1, char *string2)
{
if(!strcmp(string1,string2)){
return 1;
}
return 0;
}

int prefixCompare(char *string1, char *string2)
{
int i, length, length2;

length = strlen(string1);
length2 = strlen(string2);

if(length < length2){
length = length2;
}

for(i=0; i<=length; i++)
if(strncmp(string1,string2,i))
return i-1;
return 0;
}

int suffixCompare(char *string1, char *string2)
{
int i, length, length2;
Why not make the reverse strings the exact size you need? You get the lengths, use them
char rev1[20];
char rev2[20];

length = strlen(string1);
length2 = strlen(string2);

these are char arrays, treat them as such. IE you get random access, so length - 0 = end of the array, which could be a spot to put original char[0].

for(i=0; i <= length; i++){
sprintf(rev1,"%s%c", rev1,string1[length - i]);
}

for(i=-1; i <= length2; i++){
sprintf(rev2,"%s%c", rev2,string2[length - i]);
}

printf("\nOriginal String 1: %s\n", string1);
printf("Original String 2: %s\n", string2);
printf("Reversed String 1: %s\n", rev1);
printf("Reversed String 2: %s\n", rev2);

return prefixCompare(rev1, rev2);
}

char* commonChars(char *string1, char *string2)
{
char *pointer = strpbrk(string1, string2);

return &(*pointer);
}
[/code]

My comments in bold

[GenKiller]

you have a mistake in the second reversing bit, you use "length" where you should use "length2", but that whole bit with using sprintf seems like overkill, just use this:

Thank you _very_ much Angrybob! You were absolutely right!

I'm glad that I was correct in thinking it was the terminating byte being copied that was causing some of the problem. Thanks again!!

[GenKiller]

Prototypes go outside the main

I was never taught this, thank you for pointing this out!!!

why dont you use strcmp() from the C std libraries?

My plan (if time allots) is to remove all external function calls from the program later on and implement them within my program. My professor claims that this increases the runtime speed if you implement them yourself, and don't use the provided functions.

What is this doing? If I read the function below correct it searches for the first common character in the same position and returns that position -1, which is not something you want to test for logic. Rethink this or comment so we can understand what you want

My plan here is that if the function returns a 0, then it means that there is not a common prefix in the two strings, which means the program should continue with the next if statement and look for all common characters. If the function returns a non-zero number, then it means that there is a common prefix, and the integer returned is the number of characters in that common prefix. Is there a better way you would suggest to do this, as I believe I understand what you are saying, in by that by using the return in an if statement condition, it should be returning only a 1 or 0.

Name your methods better, this does not compare, it actually reverses. Names should be descriptive.

You are absolutely right. I started off planning on having that function do the actual suffix testing, but then came up with the idea that if I just reversed the string and passed that to the prefix() function, it would do the same thing without repeating code.

Why not make the reverse strings the exact size you need? You get the lengths, use them

Great point!!!

Thank you for your comments! You have some great points!

[Angrybob]

My plan (if time allots) is to remove all external function calls from the program later on and implement them within my program. My professor claims that this increases the runtime speed if you implement them yourself, and don't use the provided functions.

your prof is crazy, ignore him much better to having working portable easy to understand code than increase speed by 0.0001% and introduce loads more code, and bugs

You are absolutely right. I started off planning on having that function do the actual suffix testing, but then came up with the idea that if I just reversed the string and passed that to the prefix() function, it would do the same thing without repeating code.

as it stands your prefix code isn't exactly that efficient, for example you make repeated calls to the strncmp function, this would be much better done as a simple loop:

Code: Select all

int prefixCompare(char *string1, char *string2)
{
	int i, len, len2;

	len = strlen(string1);
	len2 = strlen(string2);
	
	//make len hold the smallest value
	if(len2 < len)
	{
		len = len2;
	}

	if(string1[0] != string2[0])
	{
		return 0;
	}
	
	for(i=0; i<len; i++)
	{
		if((i>0) && (string1[i] != string2[i]))
		{
			return i;
		}
	}
	return i;
}

your string reversing is also going to be a major source of inefficiency when you can simply do the reverse of the above function, but I'll let you work out the code for that

[Angrybob]

another thing to note:

in commonChars you return the address of a dereferenced pointer... but that's exactly the same as returning the pointer itself:

instead of "return &(*pointer); "
you just need "return pointer;"

or you can totally replace the inside of that function with:

return strpbrk(string1, string2);

[aja]

My professor claims that this increases the runtime speed if you implement them yourself, and don't use the provided functions.

Your professor is theoretically correct, but if your environment actually requires such marginal (I would be surprised if they were even measurable in ordinary usage scenarios) efficiency gains, you probably should be coding in assembler. Higher level languages are for portability and consistency - and re-inventing the wheel for every app begs the question as to why you are using a language that already gives you a wheel.

[GenKiller]

your prof is crazy, ignore him much better to having working portable easy to understand code than increase speed by 0.0001% and introduce loads more code, and bugs

You have no idea. He is an awful professor, both in teaching ability/style as well as overall knowledge. I ended up buying myself a book to learn it myself during my little free time, as well as a reference book.

as it stands your prefix code isn't exactly that efficient, for example you make repeated calls to the strncmp function, this would be much better done as a simple loop:

your string reversing is also going to be a major source of inefficiency when you can simply do the reverse of the above function, but I'll let you work out the code for that

Good call! My professor last semester would have strangled me if she saw that in my code! Thanks for pointing that out to me!

[not_registered]

IF you reverse the strings in-place you will save whopping amounts of memory. And as long as you are assured that each string is NULL terminated you don't have to use strlen and just rely on '\0', improving typing efficiency by a massive 0.000000000000000000000000000000016!!!