Whats wrong?

[jturner38]

Can anyone help me find out whats wrong with this? It builds successfully but when you try to use it does something unusual.




Write a program that uses function strcmp to compare two strings input by the user. The program should state whether the first string is less than, equal to or greater than the second string.

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

int main() {

	char s1[100];
	char s2[100];
	
	printf("Input first string:");
	gets(s1);

	printf("Input second string:");
	gets(s2);

	while(s1 != NULL){
		 strcmp(s1,s2);
	if(strcmp == 0)
		printf("These strings are equal");
	if(strcmp > 0)
		printf("The first string is larger");
	if(strcmp < 0)
		printf("The second string is larger");
}
	s1 == "\0";
	s2 == "\0";

	return 0;
}

[BEN10]

i dont know why you're using a loop to simply compare two strings.btw in the if conditions you're checking for strcmp to be ==0 or >0 or <0.instead the returned value(value returned by strcmp should be checked).
definition of strcmp(s1,s2) is

compare string s1 to string s2;return <0 if s1<s2,0 if s1==s2,or >0 if s1>s2

so your program should be like this.

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

int main() {

	char s1[100];
	char s2[100];
	int x;
	printf("Input first string:");
	gets(s1);

	printf("Input second string:");
	gets(s2);

	 x=strcmp(s1,s2);
	if(x == 0)
		printf("These strings are equal");
	if(x > 0)
		printf("The first string is larger");
	if(x < 0)
		printf("The second string is larger");
	s1 == "\0";
	s2 == "\0";
	return 0;
}

also dont use gets for inputting strings instead use fgets.see faq.

[jturner38]

Ok but it stills does this weird thing where it doesnt show me what I want cuz it does stuff like the matrix.

[creeping death]

Ok but it stills does this weird thing where it doesnt show me what I want cuz it does stuff like the matrix.

You have the look of a man who accepts what he sees because he is expecting to wake up. Ironically, that's not far from the truth. - Morpheus

remove the while loop and change your code to like this

//remove this	while(s1 != NULL){
//remove this		 strcmp(s1,s2);
	if((strcmp(s1,s2)) == 0)
		printf("These strings are equal");
	if((strcmp(s1,s2)) > 0)
		printf("The first string is larger");
	if((strcmp(s1,s2)) < 0)
		printf("The second string is larger");
}

...and you will have defeated the matrix

and never use gets() function

[laserlight]

remove the while loop and change your code to like this

Refer to BEN10's post #2 for a better example. There is no point calling strcmp() thrice, especially when it runs in time proportional to the length of the shorter of the strings involved. I would further change it to:

x = strcmp(s1, s2);
if (x == 0)
    printf("These strings are equal");
else if (x > 0)
    printf("The first string is larger");
else
    printf("The second string is larger");

Additionally, these two lines have no net effect:

s1 == "\0";
s2 == "\0";

[creeping death]

Refer to BEN10's post #2 for a better example. There is no point calling strcmp() thrice, especially when it runs in time proportional to the length of the shorter of the strings involved. I would further change it to:

x = strcmp(s1, s2);
if (x == 0)
    printf("These strings are equal");
else if (x > 0)
    printf("The first string is larger");
else
    printf("The second string is larger");

Additionally, these two lines have no net effect:

s1 == "\0";
s2 == "\0";

i woundn't be too worried about both of them...let the compiler perform common subexpression elimination and dead code elimination...which all standard compilers do by default.

[laserlight]

i woundn't be too worried about both of them...let the compiler perform common subexpression elimination and dead code elimination...which all standard compilers do by default.

Looking at the assembly output for a relevant sample test program compiled with the MinGW port of g++ 3.4.5 for both -O2 and -O3 optimisation levels, I'd say that the compiler has not reduced the calls of strcmp() to just one call, perhaps because it cannot prove that the calls will always return the same value.

But optimisation aside, there is a maintenance benefit to moving the function call to before the if-else chain: you only need to change the function call in one place. Likewise, there is a maintenance benefit to removing the code that has no net effect: you, and future maintainers of the code, no longer have to ask why the code is there. After all, maybe it is actually a bug that never materialised, and a helpful maintainer might just introduce the bug while trying to "fix" the non-problem. Or maybe it is not dead code after all: rather, it is a bug because the code is supposed to have a net effect.

[vart]

i woundn't be too worried about both of them...let the compiler perform common subexpression elimination and dead code elimination...which all standard compilers do by default.

How could compiler know that strings are not changed between strcmp calls?
for example by another thread?

[jturner38]

This is what my program looks like now! It works but still does that funky matrix thing. Im tryin to figure out what is causing this.

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

int main() {

	char s1[100];
	char s2[100];
	int x;
	
	printf("Input first string:");
	

	printf("Input second string:");
	
	x = strcmp(s1, s2);

	if (x == 0)
		printf("These strings are equal");
	else if (x > 0)
		printf("The first string is larger");
	else
		printf("The second string is larger");

	s1 == "\0";
	s2 == "\0";

	return 0;
}

[Sharke]

What funky Matrix thing? Your code doesn't even get input from the user now! In fact I would guess that the output was something like this:

Input first string:Input second string:The second string is larger

Not very Matrix like.

[charlitos]

lol, here is the light Neo:

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

#define MAX 100

int main() {

	char s1[MAX] = {0};
	char s2[MAX] = {0};
	int x;
	
	printf("Input first string:\n");
	fgets (s1 , MAX , stdin);

	printf("Input second string:\n");
	fgets (s2 , MAX , stdin);

	x = strcmp(s1, s2);

	if (x == 0)
		printf("These strings are equal\n");
	else if (x > 0)
		printf("The first string is larger\n");
	else
		printf("The second string is larger\n");

	return 0;
}

[Jesdisciple]

J, whenever you see strange output you should copy it from the terminal to your post rather than try to describe it... A picture paints a thousand words; while we aren't talking about pictures, the same principle applies here.

[$l4xklynx]

J, whenever you see strange output you should copy it from the terminal to your post rather than try to describe it... A picture paints a thousand words; while we aren't talking about pictures, the same principle applies here.

I agree, it would be better if you could provide us the output that you're having for the people around to see what's actually happening.

btw, fgets is also including the newline in your string so it would be better if you remove it and replace it with a nul terminator.

Ex.

fgets (s1 , MAX , stdin);

s1[ strlen( s1 ) - 1 ] = '\0';

And I don't think that you can initialize an array in C like that...

	char s1[MAX] = {0};
	char s2[MAX] = {0};

[laserlight]

btw, fgets is also including the newline in your string so it would be better if you remove it and replace it with a nul terminator.

Yes, but the newline would not be present if the input fills the entire array (save for the last char). As such, it would be better to use strchr(), e.g.,

if (fgets(s1, MAX, stdin))
{
    char *newline = strchr(s1, '\n');
    if (newline)
    {
        *newline = '\0';
    }
}

And I don't think that you can initialize an array in C like that...

Yes, you can, but in this case it would be slightly more descriptive to write:

char s1[MAX] = "";
char s2[MAX] = "";

[$l4xklynx]

if (fgets(s1, MAX, stdin))
{
    char *newline = strchr(s1, '\n');
    if (newline)
    {
        *newline = '\0';
    }
}

I see, but why put fgets as an expression in the if condition?

Yes, you can, but in this case it would be slightly more descriptive to write:

char s1[MAX] = "";
char s2[MAX] = "";

yes they can but it's wrong in C, that's what I meant... But in his program I don't think that he needs to initialize the array.