Program to Check If string is Palindrome (in C) [closed]

Question

Just wrote a program to check if a string is Palindrome. If that's true, return 1. Else, return 0. How can I improve it?

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

int palindrome(char *s)
{
    int i = 0, length = strlen(s) - 1, n = length;
    int boolean = 0;

    for (; i <= length && n >= 0; i++, n--) {
        if (toupper(s[i]) == toupper(s[n]))
            boolean = 1;
        else
            boolean = 0;
    }

    return boolean;
}

int main(void)
{
    char string[] = "Racecar";

    printf("%d", palindrome(string));

    return 0;
}

Answer 1

Bug

Given the input "racebar", the function returns true. The problem is that your boolean variable is reset after each character pair, so you are only really testing the first character with the last. I would do the following:

1. Get rid of the boolean variable and return false on the first mismatch.
2. Fix the loop iteration to only iterate through half the string instead of the whole string.
3. Mark the input parameter as const, since you do not modify the input string. This allows your function to accept both const strings and non-const string as arguments.
4. In C99, you can declare loop variables inside the loop itself, which helps to reduce the scope of these variables to only where they are used.

Here is a rewrite showing all of the above:

int palindrome(const char *s)
{
    for (int i = 0, n = strlen(s) - 1; i < n; i++, n--) {
        if (toupper(s[i]) != toupper(s[n]))
            return 0;
    }

    return 1;
}

Answer 2

Main suggestion

I would actually say length = strlen(s) instead of length = strlen(s) - 1.

This way, the variable's name reflects the variable's contents. For instance, strlen("abc") is 3, so it is counter-intuitive to set length to 2!

Of course you then need to set n = length - 1 and change your comparison to i < length. This is a very natural way to do things in C. The following is a very common pattern:

int i;
int num_items = 5;

for (i = 0; i < num_items; i++) {
    //will loop 5 times
}

Minor suggestions

• Consider renaming i and n to something more meaningful like left and right

• Use constants for true and false to make your code easier to understand at a glance

  #define TRUE 1
  #define FALSE 0
  
  //then you can return TRUE; or return FALSE;
  

• Use assertions for quick and easy testing:

  #include <assert.h>
  
  ...
  
  void testPalindrome() {
      assert(palindrome("") == TRUE);
      assert(palindrome("a") == TRUE);
      assert(palindrome("aA") == TRUE);
      assert(palindrome("ab") == FALSE);
      assert(palindrome("aba") == TRUE);
      assert(palindrome("aab") == FALSE);
      assert(palindrome("Abba") == TRUE);
      //add several more
      //try to think of cases that are likely to produce
      //false positive or false negative results
  }
  
  int main(void)
  {
      testPalindrome();
  
      //put the rest of your code here
      ...
  }
  

  This way as you make changes or improvements to your code, your tests will automatically run each time you run your program and you will notice if you accidentally break something. It also encourages you to think about how your function is actually supposed to behave. (E.g. is "" actually a palindrome? What about examples containing non-letters?)

Answer 3

As JS1 said, the code returns true for "racebar". That's because your code sets boolean to false when it sees that 'b', but overwrites it with true with the next 'a'.

I'd suggest some things:

• Don't use boolean as a name. There's a bool keyword in C++ for Boolean variables and boolean keyword in Java. Use something that indicates the contents - such as isPalindrome. Same goes for char string[]; use something like str instead.

• You don't need both i and n - due to the simple reason that the length of the string never changes. When it will change, you will need two variables to keep track of the front and back. (You'll see this when you learn about linked lists).

• To get over the overwriting problem, use break. It'll get you out of the for loop as soon as you detect that 'b', so you can return a false right away.

Here's my code:

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

#define TRUE 1
#define FALSE 0

int checkPalindrome(char * input);

int main()
{
    int res = checkPalindrome("racebar");
    if(res) printf("True\n");
    else printf("False\n");  /* this is printed */
    return 0; /* don't need this if void main()*/
}

int checkPalindrome(char * input)
{
    int i, length = strlen(input), isPal = TRUE;
    for(i = 0; i < length/2; i++)
    {
        if(input[i] != input[length - 1 - i])
        {
            isPal = FALSE;
            break;
        }
    }
    return isPal;
}

• I've used a declaration, as I've written the checkPalindrome() method after main(). It's not needed in your case, but it's a good practice to write it regardless.

• My code does not check for lower and uppercase. To add that functionality, I'd also simply call toUpper().

• A small optimization I've done is to only iterate through half the string. If it's odd, the middle character doesn't need to be checked anyway.

• By default, isPal is set to TRUE. If the string is a palindrome, the loop exits normally and TRUE is returned. Otherwise, break exits the loop abruptly and FALSE is returned.

Answer 4

When you mentally check a word to see if it's a palindrome do you first compare the letters at each end and work inwards letter by letter or do you just read the word backwards and compare to the original word? I do the latter so modelling that you could just reverse the word and compare to the original.