Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;
}
share|improve this question

closed as off-topic by Emily L., SuperBiasedMan, RubberDuck, Ethan Bierlein, Vogel612 Feb 1 at 17:23

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Emily L., SuperBiasedMan, RubberDuck, Ethan Bierlein, Vogel612
If this question can be reworded to fit the rules in the help center, please edit the question.

    
Because I posted a similar snippet myself, I can tell you that this general approach doesn't work with multi-byte encodings e.g. UTF-8, because a two-byte character would be read second-half first when going right-to-left. Just something to think about. – Sam M Jan 31 at 23:45
    
The code is obviously broken. – Emily L. Feb 1 at 10:18
    
Can you test, please, the value of palindrome("dead") ...? – CiaPan Feb 1 at 12:47
    
You might consider removing all the spaces from a string before doing the palindrome test. The user could accidentally enter a space before or after a word, and there are palindrome sentences that you might want to validate (Euston saw I was not Sue) and spaces don't count in that case. Plus, it's extra practice: What's the easiest way to remove all occurrences of a given character (space) from a string? – JPhi1618 Feb 1 at 15:03
up vote 12 down vote accepted

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;
}
share|improve this answer
    
Thanks a lot! Very helpful! – Lúcio Cardoso Jan 31 at 22:35
2  
Any reason for still defining i ahead of the loop? It's never used anywhere outside. – Zano Jan 31 at 22:59
2  
@Zano Because unless anyone says otherwise, I always assume C89. – JS1 Jan 31 at 23:40
    
Why don't you initialize i inside the for loop like so for (i = 0; instead of doing it outside and then doing for (;? – Insane Jan 31 at 23:50
1  
@Zano and JS1: I'd certainly suggest C99 syntax when it brings readability improvements. Questions that need compatibility with an ancient version of the language should ask for it (or just not adopt any recommendations that they can't use with C89). gcc 5 finally changed the default to compile C99 if you don't use any -std option. (The default is now -std=gnu11, which is C11 with GNU extensions, not just C99). I'm sure there are still some niche C89-only compilers for microcontrollers and stuff, but actively avoiding C99's better syntax is silly 16 years later. – Peter Cordes Feb 1 at 0:44

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?)

share|improve this answer
1  
Welcome to Code Review! GOod job on your first answers. – SirPython Feb 1 at 1:06

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.

share|improve this answer

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.

share|improve this answer
    
That's a sub-optimal approach to the problem. That means creating multiple copies of the string and doing a bunch of work that doesn't need to be done. – RubberDuck Feb 1 at 12:57
    
That would require preparing a reversed copy of the input, and then comparing it to the original. The latter requires the same amount of work as comparing letter by letter from both ends toward the midpoint, so the former is a pure waste of effort. – CiaPan Feb 1 at 12:59

Not the answer you're looking for? Browse other questions tagged or ask your own question.