strchr not working in C

Question

So right now I'm trying to code a program in C that takes a string and checks for proper punctuation (e.g. ends with '.', '?', or '!'). I am trying to use the strchr function to test and see if the last character in the string is one of the punctuation marks using a if loop within a for loop. However when I run the program it seems to skip the if loop all together.

Here is the program:

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

int main(void)
{
 char string[1000];
 int i,length,a,p,q,e;

 printf("Please enter a sentence with a valid punctuation.\n\n");


 for(i=0;i<1000;i++)
 {
  fgets(string,2,stdin);
  p=strchr(string,'.');
  q=strchr(string,'?');
  e=strchr(string,'!');
  if(string[sizeof(string)-1]=='.'||'?'||'!')
  {
   printf("\nYay a sentence!");
   break;
  }
  else if((p && q && e)==NULL)
  { 
   printf("You didn't provide any punctuation. Goodbye.");
   exit(a);
  }
 }
 printf("You entered the sentence:\n %s",string);

 return 0;
}

I have tried it so many different ways such as trying strstr instead or even storing it a different way through gets (which I quickly learned through gcc and some research is not the way to go.)

I am just completely lost on why this isn't working when I enter a sentence without punctuation.

Sorry if this is really simple, I am fairly new to this.

Thanks in advance.

Possible duplicate of [Why does strchr take an int for the char to be found?](http://stackoverflow.com/questions/2394011/why-does-strchr-take-an-int-for-the-char-to-be-found) — gsamaras, Oct 17 '15 at 00:19

Sergey Kalinichenko · Accepted Answer · 2015-10-17T01:48:07.843

2

You misunderstood the return value of strchr: it does not return an index of the character; instead, it returns a pointer to the character that you search.

char *p=strchr(string, '.');
char *q=strchr(string, '?');
char *e=strchr(string, '!');

In addition, sizeof does not return the actual length of the string; it returns 1000, which is the size of the string array. You need to use strlen instead.

Finally, string[strlen(string)-1]=='.'||'?'||'!' does not compare the last character to one of three characters. It always returns 1, because character codes of ? and ! are not zero, and so the logical OR || operator treats them as true value.

Same goes for (p && q && e)==NULL) condition: it does not check that all three values are NULL; one of them being NULL would be sufficient to produce an equality, but it's not what you want.

Here is how you fix this:

char last = string[strlen(string)-1];
// Skip '\n's at the end
while (last != 0 && (string[last] == '\n' || string[last] == '\r')) {
    last--;
}
if (last == '.' || last == '?' || last == '!') {
    ...
}
// Using implicit comparison to NULL is idiomatic in C
if (!p && !q && !e) {
    ...
}

edited Oct 17 '15 at 01:48

answered Oct 17 '15 at 00:19

Sergey Kalinichenko

714,442
84
1,110
1,523

Thank you! Also, is it ok to initiate *p,*q, and *e before I use fgets to get the string? Or do I actually have to have the string first before I can use those statements? – JMWBZ Oct 17 '15 at 00:32
@JMWBZ You need to have the string before you call `strchr`. You can move the declaration into the loop, and combine it with the call to `strchr`, because you do not use `p`, `q`, and `e` outside the loop. – Sergey Kalinichenko Oct 17 '15 at 00:38
@dasblinkenight I tried all of your suggestions but it still does not work. Even if I enter a sentence without any punctuation on the end, it does not print the "goodbye" statement and continues on. Any other suggestions as to why it might not work? For that matter neither the if or if else portion work, it still skips the entire loop – JMWBZ Oct 17 '15 at 01:21
@JMWBZ That's because `fgets` places `'\n'` at the end of the buffer. You need to walk back the string from the end to find punctuation. – Sergey Kalinichenko Oct 17 '15 at 01:27
Sorry to pester you, but can you explain that? I get that `fgets` puts that there, so I tried changing last to -2 instead of -1 assuming that there is an additional `\n` there because the user has to hit enter. However that didn't work. I'm still a tad lost here. – JMWBZ Oct 17 '15 at 01:47
@JMWBZ On Windows you'd get `'\n'` and `'\r'`. Take a look at the edit. – Sergey Kalinichenko Oct 17 '15 at 01:48
I do not understand the `'\r'` part, but either way I am still skipping the entire for loop. Here is the snippet of it. `for(i=0;i<1000;i++) { fgets(string,2,stdin); *p=strchr(string,'.'); *q=strchr(string,'?'); *e=strchr(string,'!'); last=strlen(string)-1; while(last!=0 && string[last]=='\n') { last--; } if(string[last]=='.' || string[last]=='?' || string[last]=='!') { .... } else if(!*p && !*q && !*e) { ... } }` Also I am on lubuntu not on windows – JMWBZ Oct 17 '15 at 02:14
@JMWBZ You've got some errors there: you used dereference operator where it does not belong. Here is [a fixed code on ideone](http://ideone.com/8mB4uk). – Sergey Kalinichenko Oct 17 '15 at 02:37

score 0 · Answer 2 · answered Oct 17 '15 at 00:25

strchr returns pointer.
expressions like hoge==a||b||c are strange. You have to write them separately.

fixed code:

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

int main(void)
{
 char string[1000];
 int i,length,a;
 char *p,*q,*e;

 printf("Please enter a sentence with a valid punctuation.\n\n");


 for(i=0;i<1000;i++)
 {
  fgets(string,2,stdin);
  p=strchr(string,'.');
  q=strchr(string,'?');
  e=strchr(string,'!');
  if(string[sizeof(string)-1]=='.'||string[sizeof(string)-1]=='?'||string[sizeof(string)-1]=='!')
  {
   printf("\nYay a sentence!");
   break;
  }
  else if(p==NULL&&q==NULL&&e==NULL)
  { 
   printf("You didn't provide any punctuation. Goodbye.");
   exit(a);
  }
 }
 printf("You entered the sentence:\n %s",string);

 return 0;
}

Note:

string[sizeof(string)-1] will be indeterminate because it means string[999] and it will never be written because you tell fgets that string has only 2 elements.
a used as the argument of exit is indeterminate. Please initialize it.
length is unused.

strchr not working in C

2 Answers2