patterncppMinor
Palindrome evaluator in C++
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
```
#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
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
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
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:
The
thanks to @screwnut for suggesting to
Lastly, for longest string, you may take advantage of STL by using
put all to gather:
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.