4
\$\begingroup\$

I have the following code that transforms a string to upper/lowercase:

#include <iterator>
#include <algorithm>
#include <type_traits>
#include <vector>
#include <vector>
#include <iostream>
#include <string>
#include <locale>
#include <functional>

namespace ae
{
    template <typename StrT>
    auto toLower(StrT& str)
    {
        std::transform(str.begin(), str.end(), str.begin(), [](const auto& c)
        {
            static const auto loc{ std::locale() };
            return std::tolower(c, loc);
        });
    }

    template <typename StrT>
    auto toUpper(StrT& str)
    {
        std::transform(str.begin(), str.end(), str.begin(), [](const auto& c)
        {
            static const auto loc{ std::locale() };
            return std::toupper(c, loc);
        });
    }

    template <typename StrT>
    auto toLowerCopy(StrT& str)
    {
        auto newStr{ str };
        std::transform(str.begin(), str.end(), newStr.begin(), [](const auto& c)
        {
            static const auto loc{ std::locale() };
            return std::tolower(c, loc);
        });
        return newStr;
    }

    template <typename StrT>
    auto toUpperCopy(StrT& str)
    {
        auto newStr{ str };
        std::transform(str.begin(), str.end(), newStr.begin(), [](const auto& c)
        {
            static const auto loc{ std::locale() };
            return std::toupper(c, loc);
        });
        return newStr;
    }
}

// Example usage
int main()
{
    std::wstring my_str{ L"Test" };
    ae::toLower(my_str);
    std::wcout << my_str << std::endl;

    std::string my_str2{ "This is a test aswell" };
    auto new_str{ ae::toUpperCopy(my_str2) };
    std::cout << new_str << std::endl;

    return 0;
}

All of the 4 functions are pretty much exact copies of each other, just minor variations. I dont want to repeat myself in this way, but im unsure how to make one function that does all of this functionality. How can I improve the code?

\$\endgroup\$

3 Answers 3

1
\$\begingroup\$

What you can do is combine the two methods toLower and toUpper into one method, say transform, to which you pass the method by which to transform (std::tolower or std::toupper) as arguments. The same can be done for toUpperCopy and toLowerCopy. Passing functions allows you to be more flexible as you are now able to not only use std::tolower and std::toupper but also any other function which transforms a character.

You can combine everything into one method and use something like a boolean parameter to tell if it should copy or not but I would advise against it. The functions are short functions so they will still be readable and you keep the responsibility for copying in a separate function.

Passing the functions would look something like this:

namespace ae
{
    template <typename charT>
    auto transform(std::basic_string<charT>& str, charT charMap(charT, const std::locale&))
    {
        std::transform(str.begin(), str.end(), str.begin(), [&charMap](const auto& c)
        {
            static const auto loc{ std::locale() };
            return charMap(c, loc);
        });
    }

    template <typename charT>
    auto transformCopy(std::basic_string<charT>& str, charT charMap(charT, const std::locale&))
    {
        auto newStr{ str };
        std::transform(str.begin(), str.end(), newStr.begin(), [&charMap](const auto& c)
        {
            static const auto loc{ std::locale() };
            return charMap(c, loc);
        });
        return newStr;
    }
}

int main()
{
    std::wstring my_str{ L"Test" };
    ae::transform(my_str, std::tolower);
    std::wcout << my_str << std::endl;

    std::string my_str2{ "This is a test aswell" };
    auto new_str{ ae::transformCopy(my_str2, std::toupper) };
    std::cout << new_str << std::endl;

    return 0;
}

Note that I changed the template variable from StrT to charT as the charT was needed for the charMap. Using both StrT and charT, the compiler was not able to infer the template argument for charT so you had to manually add the template arguments each time you called ae::transform.

\$\endgroup\$
1
\$\begingroup\$

A way to shorten your code is to extract common parts — the tolower and toupper closures and general transform and transform_copy utilities, in this case: (note that I used std::locale::classic() instead of the recently installed global locale)

namespace ae {
    struct to_lower_t {
        template <typename CharT>
        CharT operator()(CharT c) const
        {
            return std::tolower(c, std::locale::classic());
        }
    };
    inline constexpr to_lower_t to_lower;

    struct to_upper_t {
        template <typename CharT>
        CharT operator()(CharT c) const
        {
            return std::toupper(c, std::locale::classic());
        }
    };
    inline constexpr to_upper_t to_upper;

    template <typename CharT, typename Traits, typename Functor>
    void transform(std::basic_string<CharT, Traits>& str, Functor&& functor)
    {
        for (auto& c : str) {
            c = std::invoke(functor, c);
        }
    }

    template <typename CharT, typename Traits, typename Functor>
    auto transform_copy(std::basic_string_view<CharT, Traits>& str, Functor&& functor)
    {
        std::basic_string result{str};
        transform(str, functor);
        return result;
    }
}

Now the converter functions can be provided as wrappers around these.

Also, please apply remove_if(std::not_fn(needed)), sort, and unique to these include directives:

#include <iterator>
#include <algorithm>
#include <type_traits>
#include <vector>
#include <vector>
#include <iostream>
#include <string>
#include <locale>
#include <functional>
\$\endgroup\$
0
\$\begingroup\$
  1. Passing (most likely) chars by const-ref is, generally, inefficient.

  2. In-place can be done in-place, i.e. via std::for_each, or a range-for.

\$\endgroup\$
1
  • \$\begingroup\$ In what way is it inefficient? \$\endgroup\$ Apr 7, 2020 at 21:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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