HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Palindrome evaluator in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
palindromeevaluatorstackoverflow

Problem

I started learning C++ a few days ago, and this is my first full program. It takes in words until an end-of-file character prompt is given, and then outputs which of those words are palindromes and the longest palindrome of those words.

I want to know if my evaluation for whether each string is a palindrome is acceptable given the little knowledge I have about C++ thus far, and by acceptable I mean like not costly or just not agreeable for good C++ code. I was unsure if it would be better to access characters by position in each string with like a "double iterator" to check if they were the same while the iterators converged, or if just making a reversed string and checking is a better approach like I did.

Also, just if my general program structure looks okay - I'm rather unsure if my desire to have void functions to clean up my main function is really a good thing to do or otherwise.

```
#include
#include
#include

using std::cin; using std::cout; using std::string;
using std::istream; using std::list; using std::endl;
using std::setw;

istream& read(istream& is, list& word_list)
{
word_list.clear();
string word;
while(is >> word)
word_list.push_back(word);

is.clear();
return is;
}

void evaluate(list& words, string& longest)
{
longest.clear();
typedef string::size_type str_sz;

list::iterator it = words.begin();
while (it != words.end())
{
if (*it == string(it->rbegin(), it->rend()))
{
longest = max(longest, *it);
++it;
}
else
{
it = words.erase(it);
}
}
}

void output(const list& palindrs, const string& longest)
{
if (palindrs.empty())
cout ::const_iterator it = palindrs.begin();
it != palindrs.end(); ++it)
cout words;

cout << "Please enter some words." << endl;
read(cin, words);

string longest;
evaluate(words, longest);

output(words

Solution

Your code looks so good, it is quite impressive of you were learning c++ few days ago. i'm sure you will get a decent code-review but here my humble attempt.

It always preferred to pass the std::string by const reference if you don't modify it. also, std::vector is preferred over std::list. here link for more details

In your read function. It redirects all input into this container until EOF is received. However, the used container might need to reallocate memory too often, or you will end with a std::bad_alloc exception when your system gets out of memory. In order to solve these problems, you could reserve a fixed amount N of elements and process these amount of elements. here an alternative example to your read function by using std::copy_n and std::istream_iterator like so,

words.reserve(N);
std::copy_n(std::istream_iterator(std::cin),
    N,
    std::back_inserter(words));


In your evaluate function, it is fine but not efficient doe to the call of std::string constructor in every iteration along with deleting un-palindrome elements. it is better to create another container for palindrome elements and check the main container elements if it has palindrome element. possible implementation:

std::vector palindromes;
for (const auto& word : words)
{
    if(is_palindrome(word))
        palindromes.emplace_back(word);
}


The is_palindrome function, we test the given string for whither it is palindrome or not, like so:

bool is_palindrome(const std::string& str)
{
    if (str.empty())
        return false;

    using str_sz = std::string::size_type;
    for (str_sz i = 0, j = str.length() -1; i < j ; ++i, --j)
    {
        if (str[i] != str[j])
            return false;
    }
    return true;
}


thanks to @screwnut for suggesting to std::copy_if instead of for-range while the for-loop is bit clear and simple. but std::copy_if will allow us to use lambda instead of calling is_palindrome function, that might be helpful if you simply write it like this:

std::copy_if(words.begin(), words.end(), 
             std::back_inserter(palindromes), 
             [](const std::string& str)
             {return std::equal(str.begin(), str.begin() + str.size() / 2, str.rbegin());});


Lastly, for longest string, you may take advantage of STL by using std::max_element which is return iterator.

std::max_element(palindromes.begin(), palindromes.end(),
    [](const std::string& lhs, const std::string& rhs)
{
    return lhs.size() < rhs.size();
});


put all to gather:

#include 
#include 
#include 
#include 

int main()
{
    const std::size_t TotalWords = 3;
    std::vector words;
    words.reserve(TotalWords);

    std::cout  in(std::cin);
    std::copy_n(in, TotalWords, std::back_inserter(words));

    std::vector palindromes;

    auto comp = [](const std::string& str)
    {
        return std::equal(str.begin(), str.begin() + str.size() / 2, str.rbegin());
    };

    std::copy_if(words.begin(), words.end(), std::back_inserter(palindromes), comp);

    if (palindromes.empty())
    {
        std::cout  out(std::cout, "\n ");
        std::cout << "Palindromes: \n ";
        std::copy(palindromes.begin(), palindromes.end(), out);
        std::cout << "\rLongest: " << *longest << std::endl;
    }
}

Code Snippets

words.reserve(N);
std::copy_n(std::istream_iterator<std::string>(std::cin),
    N,
    std::back_inserter(words));
std::vector<std::string> palindromes;
for (const auto& word : words)
{
    if(is_palindrome(word))
        palindromes.emplace_back(word);
}
bool is_palindrome(const std::string& str)
{
    if (str.empty())
        return false;

    using str_sz = std::string::size_type;
    for (str_sz i = 0, j = str.length() -1; i < j ; ++i, --j)
    {
        if (str[i] != str[j])
            return false;
    }
    return true;
}
std::copy_if(words.begin(), words.end(), 
             std::back_inserter(palindromes), 
             [](const std::string& str)
             {return std::equal(str.begin(), str.begin() + str.size() / 2, str.rbegin());});
std::max_element(palindromes.begin(), palindromes.end(),
    [](const std::string& lhs, const std::string& rhs)
{
    return lhs.size() < rhs.size();
});

Context

StackExchange Code Review Q#129624, answer score: 5

Revisions (0)

No revisions yet.