Why doesn't my strcmp actually detect the smallest between 2 words?

Question

I'm trying to write a program that finds the "greatest" and "smallest" between a set of words inputted by the user. The program should stop listening for words assign as the user inputs a 4 letters long word and I can suppose that there aren't any words longer than 20 letters.

Here's how it should work:

    Enter word: dog
    Enter word: zebra
    Enter word: rabbit
    Enter word: catfish
    Enter word: walrus
    Enter word: cat
    Enter word: fish
    Smallest word: cat
    Largest word: zebra

I tried debugging the code and I noticed that my function only runs the first "if" twice (until zebra is inputted) and then stops working. I don't really get why, every word there is smaller than zebra so it should always run. Also I don't get why the second "if" doesn't run. Here's my code:

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

    #define N 20

    int read_line(char smallest[], char largest[]);

    int main(){
      char smallest[N], largest[N];
      int check = 0;

      while(check != 4){
        check = read_line(smallest, largest);
      }

      printf("Smallest word: %s\n", smallest);
      printf("Largest word: %s\n", largest);

      return 0;
    }

    int read_line(char smallest[], char largest[]){
      char input[N];

      printf("Enter word: ");
      scanf("%s", input);

      if(strcmp(smallest, input) < 0){
        printf("Smallest is: %s was: %s\n", input, smallest);
        strcpy(smallest, input);
      }
      if(strcmp(largest, input) > 0){
        printf("Largest is: %s was: %s\n", input, largest);
        strcpy(largest, input);
      }

      return strlen(input);
    }

Answer 1

As others have pointed out there might be a problem initialization. Since the array is not initialized you don't know what's there that's why a comment was saying it is generating an undefined behaviour. In my case I run your original code inputting twice and even after zebra it continued. Anyway is always better to initialize.

Just put a scanf outside the loop and initialize both values to the first input:

---

EDIT I did not pay attention to strcmp usage because the main problem seemed to be the initalization.

From your code and comment it is clear that you are also messing with strcmp. You should have read the documentation:

int strcmp(const char *s1, const char *s2); Upon completion, strcmp() shall return an integer greater than, equal to, or less than 0, if the string pointed to by s1 is greater than, equal to, or less than the string pointed to by s2, respectively.

So strcmp(smallest, input) ) returns a value < 0 if smallest is less (before in the dictionary pages to be clear) than input. You should change to if(strcmp(smallest, input) > 0) meaning smallest is greater than input or to if(strcmp(input, smallest) < 0)

strcmp(largest, input) returns > 0 if largest is bigger than input. Your condition strcmp(largest, input) > 0 is wrong. You want the opposite: you can invert the comparison: strcmp(largest, input) < 0 or invert the inputs: strcmp(input, largest) > 0.

---

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

#define N 21

int read_line(char smallest[], char largest[]);

int main(){
  char smallest[N];
  char largest[N];
  int check = 0;
  char firstInput[N];

  printf("Enter word: ");
  scanf("%s", firstInput);
  printf("Smallest and largest initialized to: %s\n", firstInput);
  strcpy(smallest, firstInput);
  strcpy(largest, firstInput);
  while(check != 4){
    check = read_line(smallest, largest);
  }

  printf("Smallest word: %s\n", smallest);
  printf("Largest word: %s\n", largest);

  return 0;
}

int read_line(char smallest[], char largest[]){
  char input[N];

  printf("Enter word: ");
  scanf("%s", input);

  if(strcmp(smallest, input) < 0){
    printf("Smallest is: %s was: %s\n", input, smallest);
    strcpy(smallest, input);
  }
  if(strcmp(largest, input) > 0){
    printf("Largest is: %s was: %s\n", input, largest);
    strcpy(largest, input);
  }

  return strlen(input);
}

In my opinion this is the clearer initialization (and more correct) since it uses user's input and does not rely on any condition. However the code is less compact than other possible initializations since you are repeating some part of the code.

---

EDIT alternative initializations

There are other initialization methods. One is to initialize for example to the lowest letter:

char smallest[N];
for (int i=0; i<N-1; i++)
{
  smallest[i] = 'z' ;
}
smallest[N-1] = '\0';
char biggest[N];
for (int i=0; i<N-1; i++)
{
  biggest[i] = 'a' ;
}
biggest[N-1] = '\0';

In your original code this will print something like zzzzz.... and aaaa....

Another alternative is to initialize them to the numerical min and max values of the range of char types (typically -128 and 127):

#include<math.h>
...
char smallest[N] ;
char biggest[N] ;
for (int i=0; i<N-1; i++)
{
  smallest[i] = (char) (pow(2,sizeof(char)*8)/2 - 1);
}
smallest[N-1] = '\0';
for (int i=0; i<N-1; i++)
{
  biggest[i] = (char) -1*(pow(2,sizeof(char)*8)/2);
}
biggest[N-1] = '\0';

However in case in your first print will print thrash (better not to print at all ).

---

EDIT FOR scanf alternatives

When a user is inputting anything, especially a string, it is a good behaviour to test if that input conforms to what you are expecting. In C this becomes even more important since inputting a string longer than the length (actually length-1) of the array in which the string will be stored will cause Segmentation fault error and the program crashes.

There is a simple way to avoid this: instead of scanf("%s", firstInput); do, in your specific case, scanf("%20s", firstInput); i.e. the length of your input array minus 1 (N-1). The problem with this approach is that you cannot use a dynamic value: for example:

#define M 20

scanf("%Ms", firstInput); 

does not work.

According to this post you could:

#define MAX_STRING_LENGTH 20
#define STRINGIFY(x) STRINGIFY2(x)
#define STRINGIFY2(x) #x

{
  ...
  char word[MAX_STRING_LENGTH+1];     
  scanf(file, "%" STRINGIFY(MAX_STRING_LENGTH) "s", word);
  ...
}

or follow the approach of this post:

int scanner(const char *data, char *buffer, size_t buflen)
{
    char format[32];
    if (buflen == 0)
        return 0;
    snprintf(format, sizeof(format), "%%%ds", (int)(buflen-1));
    return sscanf(data, format, buffer);
}

---

NOTE

If you mix words with and without capital letters, the ordering might not work anymore: Zookeeper (fantastic animal) comes before antilope.

Answer 2

Ok, there are multiple flaws in your program:

• First of all, as other answers pointed out, you should #define N as 21, not 20, so as to leave room for the null character.

• Secondly, you should initialize your strings.

• And, most importantly, you have messed the order of your strcmp arguments. You should either pass first input and then the string you want to compare to, or change the inequality signs.

• Finally, you should initialize largest as "" so that the first character equals zero and so the string is smaller than all possible strings. And you should initialize smallest to the largest possible string (e.g. "\xFF", although that will not work if the input string starts with the non-ASCII character '\xFF'). Then, when you enter the first string, it will be copied over both buffers. However, you'd better set a variable to check for the first input string given, and add a condition to copy it to both largest and smallest. This is slightly more complex, but works with all strings.

Here is your updated code:

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

#define N 21

int read_line(char smallest[], char largest[]);

int not_first = 0;

int main(){
  char smallest[N], largest[N];
  int check = 0;

  while(check != 4){
    check = read_line(smallest, largest);
  }

  printf("Smallest word: %s\n", smallest);
  printf("Largest word: %s\n", largest);

  return 0;
}

int read_line(char smallest[], char largest[]){
  char input[N];

  printf("Enter word: ");
  scanf("%s", input);

  if(strcmp(input, smallest) < 0 || !not_first){
    printf("Smallest is: %s was: %s\n", input, (not_first)? smallest : "(none)");
    strcpy(smallest, input);
  }
  if(strcmp(input, largest) > 0 || !not_first){
    printf("Largest is: %s was: %s\n", input, (not_first)? largest : "(none)");
    strcpy(largest, input);
  }
  not_first = 1;

  return strlen(input);
}