What's wrong with this strings comparison function in C? How are strings compared?

Question

I was in an interview yesterday and asked to write a function to compare two strings, basically the same output as strcmp(). I wrote the following program and compare() function but was told wrong. The interviewer said, "You compare the strings from lower bytes to higher bytes. If it happens that string1 has smaller lower bytes but larger higher bytes, your code will output string1 is smaller than string 2 which is wrong. "

I thought when we do string comparison, we compare two strings from left to right and compare each pair of corresponding characters with their ASCII values. I've also found some source codes of strcmp() and tried a lot of cases to compare my results with those of strcmp(), so I think the program is right.

I've put my program written in the interview here. To compare, I've printed both the values of the function I wrote and strcmp(). I have to say it's not very concise.

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

int compare (char *string1, char *string2, int length1, int length2);

int main()
{

 int len1,len2;
 char *str1;
 char *str2;
 int result;
 int result1;

//Input the string1
 printf("Please input the length of string1\n");
 scanf("%d", &len1);
 str1=malloc(sizeof(char)*len1);
 printf("Please input string 1:\n");
 scanf("%s",str1);

//Input the string2
 printf("Please input the length of string2\n");
 scanf("%d", &len2);
 str2=malloc(sizeof(char)*len2);
 printf("Please input string 2:\n");
 scanf("%s",str2);

//Do comparison, Both compare() and strcmp() are used
 result=compare(str1,str2,len1,len2);
 result1=strcmp(str1,str2);
 printf("\nThe result of compare() is: %d\n",result);
 printf("The result of strcmp() is:%d\n",result1);


 return 0;
}



int compare (char *string1, char *string2,int length1, int length2)
//If string1>string2, return1; if string1<string2, return -1; if string1=string2, return 0
{
 int result=0;

// Use the shorter length to do comprison bit by bit
 int length=(length1>length2)?length2:length1;


 for(int i=0;i<length-1;i++)
 {
  if(string1[i]>string2[i])
  {
   result=1;
   printf("%d\n",result);
   break;
  }
  else if (string1[i]<string2[i])
  {
   result=-1;
   printf("%d\n",result);
   break;
  }

 }


  if(result==1)
  {
   return 1;
  }
  else if (result==-1)
  {
   return -1;
  }
  else if (length1>length2)
  {
   return 1;
  }
  else if (length1<length2)
  {
   return -1;
  }
  else
  {
   return 0;
  }

}

So can anyone tell me what is wrong in the program? Can you give me an example that the results of compare() and strcmp() are not the same?

Thanks!

Answer 1

you are passing wrong strings length or either memory allocations for str1 and str2 is wrong that causes undefined behaviour in scanf.

for example memory allocation is:

str2 = malloc(sizeof(char) * len2); 

then only len2 chars can be in array str2 (including nul char) and string length can be len2 - 1.

I would suggest do something like(read comments):

int max_lenght = 128;  // defined a constant  
str1 = malloc(max_lenght);
printf("Please input string 1:\n");
fgets(str1, max_lenght, stdin);
len1 = strlen(str1); // calculate length 

You don't need to check result value that if it is equals to -1 return -1, if 0 then return 0 ... Just do like:

int result=0, i;  // result is 0 
for(i=0; i < length-1; i++)
{
  if(string1[i] > string2[i])
  {
       result = 1; // result is 1
       break;
  }
  else if (string1[i] < string2[i])
  {
       result = -1; // result -1
       break;
  }
}
return result;  // return what is result is 
  // comparison like if(result == -1) return -1 not needed 

in-fact further simple as:

for(result=0, i=0; i < length-1; i++){
   if(string1[i] == string2[i])
       continue;  // just continue until equal 
   if(string1[i] > string2[i])
       result = 1;
   else
       result = -1;
   break;  // else break 
}
return result;