7
\$\begingroup\$

The following code runs correctly:

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

bool is_palindrom(const std::string& str)
{
    std::string temp(str);
    std::transform(temp.begin(), temp.end(), temp.begin(), ::tolower);
    return temp == std::string(temp.rbegin(), temp.rend());
}

int main()
{
    std::string str{ "KArak" };
    std::cout << is_palindrom(str) << std::endl;
    return 0;
}

Can I do this better? The constraint is to keep the declaration of the function the same and only change the implementation.

\$\endgroup\$
8
\$\begingroup\$

To the readers: I had to use ::tolower because the std:: version was causing compiler errors. I'm investigating this right now. The issue stems from std::tolower being overloaded. What is funny is that it is in header <locale>, which means that the standard library implementation includes it in some way. Rakete1111 proposed lambda version, which works great.

Yes.

Use more standard library:

The std::string provides reverse iterators, so there is no need to do any copy at all. In fact, it is possible to it with indices, but it won't look that pretty:

    return std::equal(str.cbegin(), str.cbegin() + str.size() / 2, str.crbegin(), 
                      [](unsigned char lhs, unsigned char rhs) {
                          return std::tolower(lhs) == std::tolower(rhs);
                      });

The actual version was different, thanks to Rakete1111 for a new version. There is a note in the comments from Loki and Jerry: it seems like using functions in <cctype> are dangerous and will result in undefined behavior unless the input is cast into unsigned char first.

It is also possible to refactor the ugly + str.size() / 2 into reusable function:

#include <iterator>

//some namespace
template <typename Container>
constexpr auto mid(const Container& container)
{
    auto half_distance = std::distance(std::begin(container), 
                                       std::end(container)) / 2;
    return std::next(container.begin(), half_distance);
}

Then it transforms into:

bool is_palindrome(const std::string& str) {
    return std::equal(str.cbegin(), mid(str), str.crbegin(), 
                      [](unsigned char lhs, unsigned char rhs) {
                          return std::tolower(lhs) == std::tolower(rhs);
                      });
}

Other things that could be done better:

  • Avoid std::endl. It can cause big performance issues.

  • There is a typo in the word palindrome.

  • std::boolalpha could be used to provide more user friendly output.

  • more unit tests.

  • It seems like the code is missing <cctype> header as well.

  • putting everything into a namespace. Absence of it will force users to do unqualified call, which is prone to ADL, which is sometimes very evil.

Put together:

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

//some namespace
template <typename Container>
constexpr auto mid(const Container& container)
{
    auto half_distance = std::distance(std::begin(container), 
                                       std::end(container)) / 2;
    return std::next(container.begin(), half_distance);
}

bool is_palindrome(const std::string& str) {
    return std::equal(str.cbegin(), mid(str), str.crbegin(), 
                      [](unsigned char lhs, unsigned char rhs) {
                          return std::tolower(lhs) == std::tolower(rhs);
                      });
}

int main()
{
    std::string str{ "KArak" };
    std::cout << std::boolalpha;
    std::cout << "is \"KArak\" palindrome? " << is_palindrome(str) << '\n';
    std::cout << "is \"fuf\" palindrome? " << is_palindrome("fuf") << '\n';
    std::cout << "is empty string palindrome? " << is_palindrome("") << '\n';
    return 0;
}

Live demo.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.