0

This is a palindrome checker I have made in c. It works for all inputs, whether they have punctuation or not EXCEPT when the last item is a punctuation. In this case it does not skip it and compares and then says that it is not a palindrome when in fact it is. EX(lived, devil. will not be a palindrome but lived, devil will be). 

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

#define max 180

bool is_palindrome(const char *message);

int main()
{
    char message[max+1];

    printf("Enter a message: ");
    gets(message);
    if(!*message)
     {
         printf("input error");
         return 0;
     }

     if (is_palindrome(message)) printf("Palindrome\n");
     else printf("Not a palindrome");

     return 0;
 }

 bool is_palindrome(const char *message)
 {
     char *p, *p2;
     bool palindrome = true;

     p = message;
     p2 = message;

     for(;;)
     {
         while(*p)p++;

         while(*p2)
         {
             while(!isalpha(*p)) p--;
             while(!isalpha(*p2)) p2++;

             if (toupper(*p) != toupper(*p2))
             {
              palindrome = false;
              break;
             }else
             {
                 p--;
                 p2++;
             }

         }
         break;
     }
     return palindrome;
 }
OliviaA
  • 3
  • 3
  • Don't use `gets`. Don't ever us it. It is a dangerous function, and therefore have been *removed* from the C standard. Possibly use [`fgets`](http://en.cppreference.com/w/c/io/fgets) instead. – Some programmer dude Nov 09 '17 at 01:33
  • I have been learning from the C programming a modern approach textbook by K.N. King so that's why I picked that function as it is used to read a full line. But that's good to know for future reference thank you. – OliviaA Nov 09 '17 at 01:36
  • 1
    [Why is the gets function so dangerous that it should not be used?](https://stackoverflow.com/q/1694036/995714) – phuclv Nov 09 '17 at 01:37
  • 1
    Then I suggest you throw away that book, and check [this list of good books](http://stackoverflow.com/questions/562303/the-definitive-c-book-guide-and-list). – Some programmer dude Nov 09 '17 at 01:38
  • As for your problem, I suggest you read [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) by Eric Lippert, and learn how to use a debugger to step through your code line by line. – Some programmer dude Nov 09 '17 at 01:38
  • When `p2` is pointing to the last "l" and `p` is pointing to the first "l" what happens on the next iteration? – 001 Nov 09 '17 at 01:44
  • Johnny your comment made me double check and now I see the issue. Both the pointers must be checked in the while loop otherwise one pointer will remain at the last character it can and be compared to an incorrect character. I think at least haha, but seems to have solved the problem. Thanks! – OliviaA Nov 09 '17 at 02:01
  • What is point of that `for(;;)` and unconditional `break` at the end? – Ajay Brahmakshatriya Nov 09 '17 at 04:58

1 Answers1

0

The main problem with you code is in the following lines - 

while(!isalpha(*p)) p--;
while(!isalpha(*p2)) p2++;

This skips over all the non-alphabetical characters. Which is fine and as expected. But the problem is that, it also skips over \0 which is the string terminator.

What happens is that as p2 goes ahead, it reaches the end of the string, it starts matching the . at the end. It skips that, but also skips the \0. This causes it to read beyond the string (which might be undefined behavior, if the buffer ends there) and it produces wrong result.

What you need to do is, also stop if p2 reaches the end.

So change the lines to - 

while(!isalpha(*p)) p--;
while(*p2 != '\0' && !isalpha(*p2)) p2++;
if (*p2 == '\0')
    break;

These modifications will make your code stop at the right point and will solve your error. Also the for(;;) and the unconditional break at the end are redundant. So can be removed.

DEMO on Ideone.

Ajay Brahmakshatriya
  • 8,993
  • 3
  • 26
  • 49