Palindrome string program [closed]

Question

Closed. This question is off-topic. It is not currently accepting answers.

---

Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review.

Closed 3 years ago.

Improve this question

#include <iostream>
#include <string>
using namespace std;
string reversal(string a) {
 int i = a.length();
 string reversed_string{}; 
 for (; i >= 0; --i) 
 reversed_string += a[i];
 
 
 
 return reversed_string;
}
bool palindrome(string a) {
 
 if (a == reversal(a)) {
 
 cout << "this is a palindrome " << endl;
 return true;
 }
 else if(a != reversal(a))
 {
 cout << "this is not a palindrome " << endl;
 return false;
 }
}
int main(){
}

This is my program for the palindrome program, finding the reverse of a string was the less tedious part however the palindrome function does not work ,and i don't understand why.

• Deduplicator: Missing consistent formatting and includes. Try to make it a complete compiling program...
• THE BLACKSMITH: @Deduplicator done
• Toby Speight: main() doesn't seem to call any of the functions you defined. You could omit them without changing the observable results.
• QuasiStellar: when you are paid per line of code
• Sᴀᴍ Onᴇᴌᴀ: Unfortunately this question is off-topic because this site is for reviewing working code. Please take the tour and read up at our help center. When the code works then feel free to edit the post.

Answer 1

The large amounts of whitespace don't improve readability. A single blank line between functions would be fine, and occasional single empty lines anywhere else it's necessary to separate logical groups.

Avoid using namespace std;. It can cause surprise overloads, and makes code harder to read (because it's no longer evident where identifiers come from).

reversal() could be made more efficient by using reserve() to ensure the result string has the right capacity from the start. That said, it's simpler to use the string's reverse iterators to construct the reversed string:

std::string reversed_string{a.rbegin(), a.rend()};

But we don't actually need to allocate a new string and copy into it. If we use the std::equal() algorithm, we can just pass the two iterator pairs:

#include <algorithm>

if (std::equal(a.begin(), a.end(), a.rbegin(), a.rend())

Since C++20, we have been able to use Ranges to get a reverse-view of a string and compare it:

#include <algorithm>
#include <ranges>

if (std::ranges::equal(a, a | std::ranges::reverse))

Predicate functions are normally named beginning with is_ or has_ - here, we'd write is_palindrome(). We don't need our own copy of a, so pass by const reference.

Prefer not to produce output in a predicate function - just return the result. The caller might choose to print the result, or use it for something else. When you do produce output, don't flush the stream without good reason - i.e. prefer '\n' to std::endl.

We certainly don't need to reverse a twice - the else if can be plain else. More simply, as both branches simply return true or false respectively, we can just return the comparison result:

#include <algorithm>
#include <ranges>
#include <string>
bool is_palindrome(const std::string& a)
{
 return std::ranges::equal(a, a | std::views::reverse);
}

This is still doing more work than necessary, since we only need to compare the first half of the string to the last half (continuing beyond that point is redundant). So we can just compare the first size() / 2 elements, by using a sub-range view (std::views::take).

---

Here's where we end up - no std::strings created, and no unnecessary comparisons:

#include <algorithm>
#include <ranges>
#include <string_view>
bool is_palindrome(const std::string_view a)
{
 auto const halflen = a.size() / 2;
 return std::ranges::equal(a | std::views::take(halflen),
 a | std::views::reverse | std::views::take(halflen));
}

#include <iostream>
int main()
{
 for (auto const& s: { "", "0", "abc", "racecar" }) {
 std::cout << '"' << s << "\" "
 << (is_palindrome(s) ? "is a palindrome" : "is not a palindrome")
 << '\n';
 }
}

• Deduplicator: Wouldn't auto const half = std::views::take(a.size() / 2); return std::ranges::equal(a | half, a | std::views::reverse | half); look better?
• Toby Speight: I'm not convinced that's clearer, but it's certainly an option to consider.