C++ Palindrome program always giving 0 (false) as an output problem; Where is my code wrong?

Question

The problem is that it always outputs 0 (false) as a result. Probably the problem is in the isPalindrome function, but I cannot figure where exactly. Would be grateful if someone helped.

#include <iostream>
#include <cmath>
#include <string>
using namespace std;

bool isPalindrome(string word)
{
    bool result;

    for (int i = 0; i <= word.length() - 1; i++)
    {
        if (word.at(i) == word.length() - 1)
        {
            result = true;
        }
        else
        {
            result = false;
        }
        return result;
    }
}

int main()
{
    string word1;
    int count;
    cout << "How many words do you want to check whether they are palindromes: " << flush;
    cin >> count;

    for (int i = 0; i < count; i++)
    {
        cout << "Please enter a word: " << flush;
        cin >> word1;
        cout << "The word you entered: " << isPalindrome(word1);
    }
}

What is your intention with this line: if (word.at(i) == word.length() - 1) — Vincent, Oct 12 '19 at 12:49
Also: try to analyze this problem line by line. Use a debugger so you can see exactly where the problem is. — Vincent, Oct 12 '19 at 12:52
To check if the letter at position 0 is the same as the letter at the last position. Then continue with checking the letter at position 1 and at position - 2, and so on. — zaimoff, Oct 12 '19 at 12:53
word.length -1 does not return a letter. Also by returning true or false you do not continue the loop but exit the function so this is never going to work. — Vincent, Oct 12 '19 at 12:57
What line of code do you suggest using in order to check for the first and last letter, and so on, until you have checked all of them and determined whether or not it is a palindrome? — zaimoff, Oct 12 '19 at 12:59

bloody · Answer 1 · 2019-10-12T13:19:13.923

3

Try this one:

bool isPalindrome(string word)
{
    bool result = true;
    for (int i = 0; i < word.length() / 2; i++) //it is enough to iterate only the half of the word (since we take both from the front and from the back each time)
    {
        if (word[i] != word[word.length() - 1 - i]) //we compare left-most with right-most character (each time shifting index by 1 towards the center)
        {
            result = false;
            break;
        }  
    }    
    return result;
}

edited Oct 12 '19 at 13:19

answered Oct 12 '19 at 13:05

bloody

1,131
11
17

Just as a side note: I see no advantage in having the variable `result` and using that. It would be simpler to return true or false directly, as I have done in my answer. – Andreas Wenzel Oct 12 '19 at 17:47
1

@Andreas Basically yes but - perhaps you don't know coding styles followed in some large industrial domains (e.g. MISRA). I'll put it in even more relaxed way: whenever not very inconvinient or not performance critical always keep a single `return` statement in a function. This one is about proofness from mistakes like overlooking a release of resources (suffice to mention unlocking a mutex) or whatever 'clean-up' that should be done before leaving the function. Even if clearly no such risk - it's about the safe habit of this and elegance of this (which derives from the former motivation). – bloody Oct 13 '19 at 12:48
1

@Andreas ...another motivations of such guideline is: easier maintenance (when you need to change a returned format or edit 'clean-up' section (this is actually related to what I stated above - so more safety and more convenience) and easier debugging (one exit point). I refer you to https://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement where several pros and cons are given. Of course I neither agree nor comply to strict and stubborn "always one `return`". Just pick from these what seems best for you. – bloody Oct 13 '19 at 13:38
Thanks for the information. That is quite interesting. – Andreas Wenzel Oct 14 '19 at 13:24

Vlad from Moscow · Answer 2 · 2019-10-12T13:27:03.150

In this statement

if (word.at(i) == word.length() - 1)

the right side expression of the comparison operator is never changed and have the type std::string::size_type instead of the type char. You mean

if (word.at(i) == word.at( word.length() - 1 - i ))

However there is no sense to use the member function at. You could us the subscript operator. For example

if ( word[i] == word[word.length() - 1 - i ] )

And the loop should have word.length() / 2 iterations.

Also within the loop you are overwriting the variable result. So you are always returning the last value of the variable. It can be equal to true though a string is not a palindrome.

Also the parameter should be a referenced type. Otherwise a redundant copy of the passed argument is created.

The function can be defined the following way

bool isPalindrome( const std::string &word )
{
    std::string::size_type i = 0; 
    std::string::size_type n = word.length();

    while ( i < n / 2 && word[i] == word[n - i - 1] ) i++;

    return i == n / 2;
}

Another approach is the following

bool isPalindrome( const std::string &word )
{
    return word == std::string( word.rbegin(), word.rend() );
}

Though this approach requires to create a reverse copy of the original string.

The simplest way is to use the standard algorithm std::equal. Here is a demonstrative program

#include <iostream>
#include <string>
#include <iterator>
#include <algorithm>

bool isPalindrome( const std::string &word )
{
    return std::equal( std::begin( word ), 
                       std::next( std::begin( word ), word.size() / 2 ),
                       std::rbegin( word ) );
}

int main() 
{
    std::cout << isPalindrome( "123454321" ) << '\n';

    return 0;
}

Andreas Wenzel · Answer 3 · 2020-03-17T03:38:08.440

The loop in the function isPalindrome will only execute once, because the return statement is unconditionally executed in the first iteration of the loop. I am sure that this is not intended.

To determine whether a string is a palindrome, the loop must be executed several times. Only after the last character has been evaluated (in the last iteration of the loop) will it be time to use the return statement, unless you determine beforehand that the string is not a palindrome.

Also, in the function isPalindrome, the following expression is nonsense, as you are comparing the ASCII Code of a letter with the length of the string:

word.at(i) == word.length() - 1

Therefore, I suggest the following code for the function:

bool isPalindrome(string word)
{
    for (int i = 0; i < word.length() / 2; i++)
    {
        if (word.at(i) != word.at( word.length() - i - 1) ) return false;
    }

    return true;
}

It's enough to iterate only the half of the word: `i < word.length() / 2` (since we take both from the front and from the back - towards the center). — bloody, Oct 12 '19 at 13:34
Thanks for pointing that out. I have edited my answer accordingly. — Andreas Wenzel, Oct 12 '19 at 14:36

pjaj · Answer 4 · 2019-10-13T09:59:51.840

Two possible problems.

You appear to be comparing a character to a number

if (word.at(i) == word.length() - 1)

shouldn't this be

if (word.at(i) == word.at(word.length() - i)) ?

There are 3 returns within the if statement, so no matter what the outcome it's only going to compare one character before returning to the calling function.

As a point of technique, repeated calls to .length inside the loop, which always returns the same value, wastes time and makes the code more difficult to understand.

You need to return as soon as you find a mismatch. If you are looking for a palindrome you only need to compare the first half of the word with the second half in reverse order. Something like

bool isPalindrome(string word)
{
    for (int i = 0, j= word.length() - 1; i<j; i++, j--)
    // i starts at the beginning of the string, j at the end.
    // Once the i >= j you have reached the middle and are done.
    // They step in opposite directions
    {
        if (word[i] != word[j])
        {
            return false;
        }              
    }
    return true;
}

score 0 · Answer 5 · answered Oct 12 '19 at 13:19

0

I hope this one helps you also (corrected also warnings):

bool isPalindrome(string word)
{
    bool result = false;

    int lengthWord = (int)word.length();

    for (int i = 0; i <= (lengthWord / 2); ++i)
    {
        if (word.at(i) == word.at(lengthWord - i -1))
        {
            result = true;
            continue;
        }
        result = false;
    }
    return result;
}

answered Oct 12 '19 at 13:19

neaAlex

206
3
15

You should break the loop as soon as it's obvious false. This one is inefficient. Also you don't have to re-assign true every time it's equal. It's enough to initialize it once before the loop (feel free to see my answer). – bloody Oct 12 '19 at 13:21
True enough :). Thanks for the tip. – neaAlex Oct 12 '19 at 13:26

Vincent · Answer 6 · 2019-10-12T21:24:21.733

-1

As discussed in the comments under your question. You made some mistakes in the code.

Your function should more or less look like this:

bool isPalindrome(string word) { 

    bool result = true; 

    for (int i = 0; i <= word.length() - 1; i++)
    { 
        if (word.at(i) != word.at(word.length() - 1 -i))     
        { 
            return false; 
        } 
    } 
    return result;
}

edited Oct 12 '19 at 21:24

answered Oct 12 '19 at 13:01

Vincent

2,073
1
17
24

1

It cannot work. You're always comparing with the last character. – bloody Oct 12 '19 at 13:13
1

This answer is incorrect, because it will never return true. – Andreas Wenzel Oct 12 '19 at 13:20
@Andreas Wenzel: I was sloppy. Fixed it. – Vincent Oct 12 '19 at 15:25
@bloody: See above. – Vincent Oct 12 '19 at 15:25
Yea, now it's better:) – bloody Oct 13 '19 at 13:00

C++ Palindrome program always giving 0 (false) as an output problem; Where is my code wrong?

6 Answers6