Comparing C-strings

[sirjames2004]

Hey guys I'm back again with hopefully the last chapter in my C-string saga. What's really messed up is that my compare function WORKS, just not when I get input from a user. Check out my main below. The first block is what the program's supposed to do; get the strings from a user and do stuff with them. But when they enter the same string, it does not report them as equal. Even though it DOES in the next block PERFECTLY!

This is the type of stuff why I don't think I'm cut out to be a full-time programmer

EDIT: Also, just like with my prior C-string posts, my instructor won't let us change the function headers given from the prototypes. It's like working in steal cage here

// Author: Matt
// File: mjccstring.cpp
// Operating System: Linux
// Compiler: g++
// Written: 2/2013
// Assigned: 2/7/2013
// Due: 2/15/2013
// Purpose: 	This program will take two strings as input from the user, and perform various C-String related tasks with them

#include <iostream>

//prototypes
int mjcStrLen(const char * const);
int mjcStrCmp(const char [], const char []);

//initializes variables and calls the other functions
int main()
{

	char cstring1[256];
	char cstring2[256];
	std::cout << "Please enter the text for the 1st string. Press enter when done:  ";
	std::cin.getline (cstring1, 256);
	std::cout << "Please enter the text for the 2nd string. Press enter when done:  ";
	std::cin.getline (cstring2, 256);
	std::cout << "String \"" << cstring1 << "\" is " << mjcStrLen(cstring1) << " characters long." << std::endl;
	std::cout << "String \"" << cstring2 << "\" is " << mjcStrLen(cstring2) << " characters long." << std::endl;
	std::cout << "Return vaule of mjcStrCmp is:  " << mjcStrCmp(cstring1, cstring2) << std::endl;
	if(mjcStrCmp(cstring1, cstring2) == 0)
		std::cout << "String \"" << cstring1 << "\" is equal to \"" << cstring2 << "\"." << std::endl;
	else
		std::cout << "String \"" << cstring1 << "\" is not equal to \"" << cstring2 << "\"." << std::endl;



	std::cout << std::endl;
	std::cout << "mjcStrCmp Test: Created two new strings which should be equal" << std::endl;
	char cstring3[256] = "Testing";
	char cstring4[256] = "Testing";
	std::cout << "Return vaule of mjcStrCmp is:  " << mjcStrCmp(cstring3, cstring4) << std::endl;
	if(mjcStrCmp(cstring3, cstring4) == 0)
		std::cout << "String \"" << cstring3 << "\" is equal to \"" << cstring4 << "\"." << std::endl;
	else
		std::cout << "String \"" << cstring3 << "\" is not equal to \"" << cstring4 << "\"." << std::endl;
	return 0;
}

//uses a while loop to count the number of characters in a C-string
int mjcStrLen(const char * const test_string)
{
	int length = 0;
	int index = 0;
	while(*(test_string + index)) //will be true until it hits the null character
	{
		length++;
		index++;
	}
	return length;
}

//compares ascii values of each character, returns 1 if larger, 0 if equal, or -1 if smaller
int mjcStrCmp(const char test_string1[], const char test_string2[])
{
	int return_value = 0;
	for(int index = 0; index < 256; index++)
	{
		if(test_string1[index] > test_string2[index])
		{
			return_value = 1;
			index = 256; //will skip rest of string once it finds a different character

		}
		else if(test_string1[index] < test_string2[index])
		{
			return_value = -1;
			index = 256;
		}
	}
	return return_value;
}

[Chervil]

In function mjcStrCmp(), instead of checking for a fixed number of characters (256), the testing should end when the null terminator of either string is reached.

[sirjames2004]

Made the following changes to my function. Now everything I enter is evaluating to equal (return value is '0') in both blocks of code in main

int mjcStrCmp(const char test_string1[], const char test_string2[])
{
	int return_value = 0;
	for(int index = 0; index != NULL; index++)
	{
		if(test_string1[index] > test_string2[index])
		{
			return_value = 1;
			index = NULL;
		}
		else if(test_string1[index] < test_string2[index])
		{
			return_value = -1;
			index = NULL;
		}
	}
	return return_value;
}

[LB]

for(int index = 0; index != NULL; index++)

Do you know what the NULL constant is? It's just 0. The for loop never executes.

I think you meant for the condition to loop until you reached the end of one of the strings.

[Chervil]

That doesn't look right.
The loop should end when either test_string1[index] == '\0'
or test_string2[index] == '\0'
though of course there are many ways to put that into code.

[sirjames2004]

Tried this. The first block seems to work now, but the 2nd block thinks the two literals are not equal now

int mjcStrCmp(const char test_string1[], const char test_string2[])
{
	int return_value = 0;
	for(int index = 0; test_string1[index] || test_string2[index] || return_value == 0; index++)
	{
		if(test_string1[index] > test_string2[index])
		{
			return_value = 1;
		}
		else if(test_string1[index] < test_string2[index])
		{
			return_value = -1;
		}
	}
	return return_value;
}

[LB]

for(int index = 0; test_string1[index] || test_string2[index] || return_value == 0; index++)
That condition is wrong. The condition says
"while ((the character at index index of test_string1 is not 0) OR (the character at index index of test_string2 is not 0) OR (return_value is 0))"

The condition you want should say
"while ((return_value is 0) AND (the character at index index of test_string1 is not 0) AND (the character at index index of test_string2 is not 0))"

Remember, the condition should be true while it runs - it is NOT the condition to stop at.

[sirjames2004]

Yeah my bad. Actually made a quick edit when I saw that. It still doesn't work though

[LB]

I edited my post. It didn't change much :p

[Chervil]

Putting the check for end-of-string at the start of the loop doesn't completely make sense. If one string is longer than the other, say "dog" and "dogs", you still need to enter the loop to see which string is greater or lesser.

I'd prefer to test the condition at the end of each pass through the loop.

[LB]

@Chervil what if an empty string is given?

[Chervil]

If either or both strings are empty, it's still safe to test the first character, which will contain the terminating null in at least one case.

[sirjames2004]

Thanks guys. I believe it works now :)

//compares ascii values of each character, returns 1 if larger, 0 if equal, or -1 if smaller
int mjcStrCmp(const char test_string1[], const char test_string2[])
{
	int return_value = 0;
	int index = -1;
	do
	{
		index++;
		if(test_string1[index] > test_string2[index])
		{
			return_value = 1;
		}
		else if(test_string1[index] < test_string2[index])
		{
			return_value = -1;
		}
	}
	while(test_string1[index] && test_string2[index] && return_value == 0);
	return return_value;
}

[Chervil]

That looks very similar to a version that I came up with, but some differences

int mjcStrCmp(const char one[], const char two[])
{
    int ix = -1;

    do
    {
        ix++;
        if (one[ix] > two[ix])
            return 1;

        if (one[ix] < two[ix])
            return -1;

    } while (one[ix] && two[ix]);

    return 0;
}

[sirjames2004]

Indeed, that probably would have been what I would have wrote, but unfortunately the instructor's really anal about certain things. Such as one exit/return statement for each function.

*sigh* Gonna be so glad when this semester's over, and it's still only February

[LB]

sirjames2004 wrote:

Such as one exit/return statement for each function.

Heh, that's back from in C where objects aren't automagically destructed at the end of their scope and so you would have to copy de-initialization code before every return. In C++ you don't worry about that anymore.

[Lowest0ne]

At the end of your loop, idx is not out of either array's bounds. You can therefore remove the if statements from the loop and do the calculation in the return statement:

int mystrCmp(const char str1[], const char str2[])
{
  int idx = 0; 
  while(str1[idx] && str1[idx] == str2[idx])
  {
    idx++;
  }

  return  str2[idx] > str1[idx] ? -1 : str2[idx] < str1[idx];
}

[LB]

@Lowest1 what if str2 is shorter than str1? You're accessing memory out of bounds.

[Lowest0ne]

if str2[idx] = NULL then the loop breaks because that isnt equal to whatever str1[idx] is

[sirjames2004]

@L B Yeah he's a real old-school C guy. I can tell. Cool guy personally, just really challenging. Believe it or not I've had two prior C++ courses years ago, and taking this one makes me realize I must not have learned much from the others. It's sweetly agonizing if that makes any sense >_<