Why does the first and second case keep returning false when both the original and reversed strings are the same? Palindrome code

Question

Why does the function return false for the first and second words when they are both palindromes? The reversed text should be the same as the original.

#include <iostream>
#include <string>

// Define is_palindrome() here:
bool is_palindrome(std::string text) {
    std::string reversed = "";
    for (int i = text.size(); i >= 0; i--) {
        reversed.push_back(text[i]);
    }

    if (reversed == text) {
        return true;
    }
    else {
        return false;
    }
}
    int main() {
        std::cout << is_palindrome("madam") << "\n";
        std::cout << is_palindrome("ada") << "\n";
        std::cout << is_palindrome("lovelace") << "\n";

    }

Answer 1

For starters the function parameter should be constant referenced type

bool is_palindrome( const std::string &text ) {

In this loop

for (int i = text.size(); i >= 0; i--) {
    reversed.push_back(text[i]);
}

when i is initially equal to text.size() when you are accessing a non-actual element of the string. According to the C++ 11 Standard it is equal to '\0'.

Also it is a bad idea to use the signed type int as the type of the index instead of the unsigned type std::string::size_type.

Moreover there is no need to create a copy of the whole string to determine whether the given string is a palindrome. This is just a bad approach because it is inefficient.

The function can be defined the following way

bool is_palindrome( const std::string &s ) 
{
    std::string::size_type i = 0, n = s.size();

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

    return i == n / 2;
}

Here is a demonstrative program.

#include <iostream>
#include <iomanip>
#include <string>

bool is_palindrome( const std::string &s ) 
{
    std::string::size_type i = 0, n = s.size();

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

    return i == n / 2;
}

int main() 
{
    std::cout << std::boolalpha << is_palindrome("madam") << "\n";
    std::cout << std::boolalpha << is_palindrome("ada") << "\n";
    std::cout << std::boolalpha << is_palindrome("lovelace") << "\n";

    return 0;
}

Its output is

true
true
false

Another approach is to use the standard algorithm std::equal.

Here is a demonstrative program.

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

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

int main() 
{
    std::cout << std::boolalpha << is_palindrome("madam") << "\n";
    std::cout << std::boolalpha << is_palindrome("ada") << "\n";
    std::cout << std::boolalpha << is_palindrome("lovelace") << "\n";

    return 0;
}

The program output is the same as shown above

true
true
false

If your compiler supports the C++ 17 Standard then it is even better to declare the function parameter as having the type std::string_view.

For example (or you can use the implementation with the standard algorithm std::equal)

#include <string_view>

bool is_palindrome( std::string_view s )
{    
    std::string_view::size_type i = 0, n = s.size();

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

    return i == n / 2;
}

In this case you will be able to call the function selecting a sub-string of a string without creating an object of the type std::string as for example

std::cout << std::boolalpha << is_palindrome( { "adam", 3 } ) << "\n";

Answer 2

This line is wrong

for (int i = text.size(); i >= 0; i--) {
        reversed.push_back(text[i]);
    }

Notice that you are indexing from .size(), but that is not correct. The character at position text[text.size()] is guaranteed to be \0. This is probably not part of the string whose 'palindrome-ness' you want to check.

You need to do

for (int i = text.size() - 1; i >= 0; i--) {
        reversed.push_back(text[i]);
    }

If you actually want to reverse a string, you could just use std::reverse like this

auto rev = text;
std::reverse(rev.begin(), rev.end());

and then compare with rev == text.

Note that if you just want to check if a string is a palindrome, there are much more efficient ways to do it. To start off, your example is making 2 unnecessary copies; one in the function parameter, and one to store the reversed string.

Change the signature to take by const reference, and use an algorithm

bool is_palindrome(std::string const &text) 
{
  return std::equal(text.begin(), 
                    text.begin() + text.size() / 2, 
                    text.rbegin());
}

Answer 3

Your array bounding is incorrect. In C++ indices begin at 0 so the highest index of your array will be text.size() - 1 Try this instead:

for (int i = text.size() - 1; i >= 0; i--) {
        reversed.push_back(text[i]);
}