6
\$\begingroup\$

After receiving advice from my last question, I reworked the design of my program to be more generic, expandable and easier to use. This is the excerpt of the requirements:

Step 1: Basic Title Case

For each word in a sentence, make all of its letters lower case, except the first one that would be in upper case.

There is a list of “exceptions” words that need to be entirely in lower case, including their first letter. This lists includes “at” and “to”, along with another dozen words. For the sake of this project let’s say that the list of exceptions is: a, an, the, at, by, for, in, of, on, to, and, as and or.

Note that in all cases, the first word of the string must start with a capital letter.

Step 2: Title Case with customizations

Make the list of exceptions customizable: the library user can add new exceptions, replace the list with their own, or add words that should not be changed by the library.

An additional requirement is that words in all caps should be left as is (“STL” must remain “STL”, and not be changed into “Stl”), but the library user should be able to opt-out of this feature.

Step 3: Other capitalizations

Title Case is one of the various forms of capitalisation there is. Other examples include UPPER CASE, lower case, snake_case, Start case, and so on.

Implement at least two other forms of capitalizations in the library, by reusing as much code as possible from the previous steps. The point of this step is to make a smooth introduction of new features into existing code via a form of polymorphism (runtime, static, other… you choose).

Any advice would be appreciated.

Case.h

#ifndef CASE_POLICY_H
#define CASE_POLICY_H

#include <utility>
#include <cctype>
#include <array>

namespace case_utils {

    namespace helper {
        //converts const_iterator to iterator, needed to convert result of find_next_word() without changing 
        //param to StringType& which contradicts the purpose of the function
        template <typename Container, typename ConstIterator>
        inline typename Container::iterator remove_constness(Container& c, ConstIterator it)
        {
            return c.erase(it, it);
        }
    }

    namespace defaults {
        const auto to_upper = [](int c)->char { return std::toupper(static_cast<unsigned char>(c)); };
        const auto to_lower = [](char c)->char { return std::tolower(static_cast<unsigned char>(c)); };
        const auto is_upper = [](char c)->bool { return std::isupper(static_cast<unsigned char>(c)); };
        const auto is_not_letter = [](char c)->bool {return !std::isalpha(static_cast<unsigned char>(c)); };
        const auto is_space = [](char c)->bool { return std::isspace(static_cast<unsigned char>(c)); };
    }

    template<typename StringType>
    class CasePolicy {
    public:
        using string_type = StringType;
        using iterator = typename StringType::iterator;
        using const_iterator = typename StringType::const_iterator;

        virtual std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) = 0;
        virtual void transform_word(iterator begin_word, iterator end_word, StringType& text) = 0;
        virtual bool meets_pred(const_iterator begin_word, const_iterator end_word) = 0;
    };

    template<typename StringType, typename PolicyString>
    decltype(auto) transform_text(StringType&& text, CasePolicy<PolicyString>* policy)
    {
        using std::begin;
        using std::end;

        PolicyString text_copy{ std::forward<StringType>(text) };

        auto first_word = policy->find_next_word(text_copy, begin(text_copy));

        auto begin_word = helper::remove_constness(text_copy, first_word.first);
        auto end_word = helper::remove_constness(text_copy, first_word.second);

        if (end_word == end(text_copy)) //check if there's only one word in string or string is empty
        {
            if (begin_word == end_word) return text_copy; //empty
            if (policy->meets_pred(begin_word, end_word)) {
                policy->transform_word(begin_word, end_word, text_copy);
            }
            return text_copy;
        }

        while (begin_word != end_word) // no more whitespace delimited words in the string
        {
            if (policy->meets_pred(begin_word, end_word)) {
                policy->transform_word(begin_word, end_word, text_copy);
            }

            if (end_word == end(text_copy)) break; // no more characters to observe in the string
            auto next_word = policy->find_next_word(text_copy, end_word + 1);

            begin_word = helper::remove_constness(text_copy, next_word.first);
            end_word = helper::remove_constness(text_copy, next_word.second);
        }
        return text_copy;
    }
}
#endif 

TitleCase.h

#ifndef  TITLE_CASE_H
#define TITLE_CASE_H

#include "Case.h"

#include <algorithm>
#include <array>
#include <iterator>
#include <memory>

namespace case_utils {

    namespace defaults {
        using sv = std::string_view;
        constexpr auto title_case_exceptions_list = std::array{ sv{ "a" }, sv{ "an" }, sv{ "and" }, sv{ "as" }, sv{ "at" }, sv{ "by" }, sv{ "for" }, sv{ "in" }, sv{ "of" }, sv{ "on" }, sv{ "to" }, sv{ "or" }, sv{ "the" } };
    }

    template<typename InputIterator, typename ForwardRange>
    bool is_exception(InputIterator begin_word, InputIterator end_word, const ForwardRange& exceptions)
    {
        using std::begin; using std::end;
        for (const auto& exception : exceptions)
        {
            if (std::equal(begin(exception), end(exception), begin_word, end_word))
            {
                return true;
            }
        }
        return false;
    }

    template<typename InputIterator, typename IsUpperPred>
    bool is_acronym(InputIterator begin_word, InputIterator end_word, IsUpperPred is_upper_pred)
    {
        return std::all_of(begin_word, end_word, is_upper_pred);
    }

    template<typename StringType, typename ForwardRange, typename IsWhitespace, typename IsUpperPred, typename ToUpperFunction, typename UnaryToLowerFunction>
    class TitleCasePolicy : public CasePolicy<StringType> {
        const ForwardRange& m_exceptions;
        IsWhitespace m_is_whitespace;
        IsUpperPred m_is_upper_pred;
        ToUpperFunction m_to_upper;
        UnaryToLowerFunction m_to_lower;

    public:
        using typename CasePolicy<StringType>::iterator;
        using typename CasePolicy<StringType>::const_iterator;

        TitleCasePolicy(const ForwardRange& exceptions, IsWhitespace is_white_space, IsUpperPred is_upper_pred, ToUpperFunction to_upper, UnaryToLowerFunction to_lower)
            :m_exceptions{ exceptions }, m_is_whitespace{ is_white_space }, m_is_upper_pred{ is_upper_pred }, m_to_upper{ to_upper }, m_to_lower{ to_lower } {}

        //find first whitespace-delimited word starting from position first
        std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
        {
            using std::end;

            const_iterator begin_word = std::find_if_not(first, end(text), m_is_whitespace);
            const_iterator end_word = std::find_if(begin_word, end(text), m_is_whitespace);
            return { begin_word, end_word };
        }

        void transform_word(iterator begin_word, iterator end_word, StringType&) override
        {
            *begin_word = m_to_upper(*begin_word);
            while (++begin_word != end_word) { *begin_word = m_to_lower(*begin_word); }
        }

        bool meets_pred(const_iterator begin_word, const_iterator end_word) override
        {
            return !is_acronym(begin_word, end_word, m_is_upper_pred) && !is_exception(begin_word, end_word, m_exceptions);
        }

        const ForwardRange& exceptions() { return m_exceptions; }
    };

    template<typename StringType, typename ForwardRange, typename IsWhitespace, typename IsUpperPred, typename ToUpperFunction, typename UnaryToLowerFunction>
    class TitleCasePolicyIncludingAllCaps : public TitleCasePolicy<StringType, ForwardRange, IsWhitespace, IsUpperPred, ToUpperFunction, UnaryToLowerFunction>
    {
    public:
        using Base = TitleCasePolicy<StringType, ForwardRange, IsWhitespace, IsUpperPred, ToUpperFunction, UnaryToLowerFunction>;
        TitleCasePolicyIncludingAllCaps(const ForwardRange& exceptions, IsWhitespace is_white_space, IsUpperPred is_upper_pred, ToUpperFunction to_upper, UnaryToLowerFunction to_lower)
            : Base{ exceptions, is_white_space, is_upper_pred, to_upper, to_lower } {}
        using typename CasePolicy<StringType>::const_iterator;
        bool meets_pred(const_iterator begin_word, const_iterator end_word) override
        {
            return !is_exception(begin_word, end_word, this->exceptions());
        }
    };

    //helper function similar to make_pair b/c constructors can't be used to deduce template class types
    template<typename StringType, typename ForwardRange = decltype(defaults::title_case_exceptions_list),
        typename IsWhitespace = decltype(defaults::is_not_letter), typename IsUpperPred = decltype(defaults::is_upper),
        typename ToUpperFunction = decltype(defaults::to_upper), typename UnaryToLowerFunction = decltype(defaults::to_lower)>
    inline auto make_title_case_policy(bool ignore_acronyms = true, const ForwardRange& exceptions = defaults::title_case_exceptions_list,
            IsWhitespace is_white_space = defaults::is_not_letter, IsUpperPred is_upper_pred = defaults::is_upper,
            ToUpperFunction to_upper = defaults::to_upper, UnaryToLowerFunction to_lower = defaults::to_lower)
        ->std::unique_ptr<TitleCasePolicy<StringType, ForwardRange, IsWhitespace, IsUpperPred, ToUpperFunction, UnaryToLowerFunction>>
    {
        using TCP = TitleCasePolicy<StringType, ForwardRange, IsWhitespace, IsUpperPred, ToUpperFunction, UnaryToLowerFunction>;
        using TCPIAC = TitleCasePolicyIncludingAllCaps<StringType, ForwardRange, IsWhitespace, IsUpperPred, ToUpperFunction, UnaryToLowerFunction>;

        if (ignore_acronyms)
        {
            return std::make_unique<TCP>(exceptions, is_white_space, is_upper_pred, to_upper, to_lower);
        }
        else
        {
            return std::make_unique <TCPIAC>(exceptions, is_white_space, is_upper_pred, to_upper, to_lower);
        }
    }

    template<typename StringType, typename ForwardRange>
    inline decltype(auto) title_case(StringType&& text, bool ignore_acronyms, const ForwardRange& exceptions)
    {
        using string_type = typename std::decay_t<StringType>;
        static auto policy = make_title_case_policy<string_type>(ignore_acronyms, exceptions);
        return transform_text(std::forward<StringType>(text), policy.get());
    }

    template<typename StringType>
    inline decltype(auto) title_case(StringType&& text)
    {
        return title_case(std::forward<StringType>(text), true, defaults::title_case_exceptions_list);
    }

    template<typename StringType, typename ForwardRange>
    inline decltype(auto) title_case(StringType&& text, const ForwardRange& exceptions)
    {
        return title_case(std::forward<StringType>(text), true, exceptions);
    }

    template<typename StringType>
    inline decltype(auto) title_case(StringType&& text, bool ignore_acronyms)
    {
        return title_case(std::forward<StringType>(text), ignore_acronyms, defaults::title_case_exceptions_list);
    }
}
#endif

UpperCase.h

#ifndef UPPER_CASE_H
#define UPPER_CASE_H

#include "Case.h"

#include <memory>
#include <iterator>
#include <utility>

namespace case_utils {
    template<typename StringType, typename ToUpper>
    class UpperCasePolicy : public CasePolicy<StringType>
    {
        ToUpper m_to_upper;
    public:
        using typename CasePolicy<StringType>::iterator;
        using typename CasePolicy<StringType>::const_iterator;

        UpperCasePolicy(ToUpper to_upper) : m_to_upper{ to_upper } {}

        std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
        {
            using std::begin; using std::end;
            return { begin(text), end(text) };
        }

        void transform_word(iterator begin_word, iterator end_word, StringType&) override
        {
            while (begin_word != end_word)
            {
                *begin_word = m_to_upper(*begin_word);
                ++begin_word;
            }
        }

        bool meets_pred(const_iterator begin_word, const_iterator end_word) override
        {
            return true;
        }
    };

    template<typename StringType, typename ToUpper = decltype(defaults::to_upper)>
    inline auto make_upper_case_policy(ToUpper to_upper = defaults::to_upper) ->std::unique_ptr<UpperCasePolicy<StringType, ToUpper>>
    {
        using UPC = UpperCasePolicy<StringType, ToUpper>;
        return std::make_unique<UPC>(to_upper);
    }

    template<typename StringType, typename ToUpper>
    inline decltype(auto) upper_case(StringType&& text, ToUpper to_upper)
    {
        using string_type = typename std::decay_t<StringType>;
        static auto policy = make_upper_case_policy<string_type>(to_upper);
        return transform_text(std::forward<StringType>(text), policy.get());
    }

    template<typename StringType>
    inline decltype(auto) upper_case(StringType&& text)
    {
        return upper_case(std::forward<StringType>(text), defaults::to_upper);
    }
}

#endif

SnakeCase.h

#ifndef SNAKE_CASE_H
#define SNAKE_CASE_H

#include "Case.h"

#include <iterator>
#include <memory>
#include <utility>

namespace case_utils {

    template<typename StringType, typename CharT, typename IsWhitespace, typename ToLower>
    class SnakeCasePolicy : public CasePolicy<StringType> {
    private:
        CharT m_underscore;
        IsWhitespace m_is_whitespace;
        ToLower m_to_lower;
    public:
        using typename CasePolicy<StringType>::iterator;
        using typename CasePolicy<StringType>::const_iterator;

        SnakeCasePolicy(CharT underscore, IsWhitespace is_whitespace, ToLower to_lower)
            : m_underscore{ underscore }, m_is_whitespace{ is_whitespace }, m_to_lower{ to_lower } {}

        std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
        {
            using std::begin; using std::end;
            const_iterator begin_word = std::find_if_not(begin(text), end(text), m_is_whitespace);
            const_iterator end_word = std::find_if(begin_word, end(text), m_is_whitespace);
            return { begin_word, end_word };
        }

        void transform_word(iterator begin_word, iterator end_word, StringType& text) override
        {
            while (begin_word != end_word)
            {
                *begin_word = m_to_lower(*begin_word);
                ++begin_word;
            }
            if (end_word != end(text)) 
            {
                transform_whitespace(end_word, text);
            }
        }

        void transform_whitespace(iterator end_word, StringType& text)
        {
            using std::end;
            *end_word = m_underscore;
            ++end_word; //beginning of whitespace
            auto end_whitespace = std::find_if_not(end_word, end(text), m_is_whitespace);
            text.erase(end_word, end_whitespace);
        }

        virtual bool meets_pred(const_iterator begin_word, const_iterator end_word)
        {
            return true;
        }
    };

    template<typename StringType, typename CharT = char, typename IsWhitespace = decltype(defaults::is_space), 
        typename ToLower = decltype(defaults::to_lower)>
    inline auto make_snake_case_policy(CharT underscore = '_', IsWhitespace is_whitespace = defaults::is_space,
            ToLower to_lower = defaults::to_lower)->std::unique_ptr<SnakeCasePolicy<StringType, CharT, IsWhitespace, ToLower>>
    {
        using SCP = SnakeCasePolicy<StringType, CharT, IsWhitespace, ToLower>;
        return std::make_unique<SCP>(underscore, is_whitespace, to_lower);
    }

    template<typename StringType, typename CharT, typename IsWhitespace, typename ToLower>
    inline decltype(auto) snake_case(StringType&& text, CharT underscore, IsWhitespace is_whitespace, ToLower to_lower)
    {
        using string_type = typename std::decay_t<StringType>;
        static auto policy = make_snake_case_policy<string_type>(underscore, is_whitespace, to_lower);
        return transform_text(std::forward<StringType>(text), policy.get());
    }

    template<typename StringType>
    inline decltype(auto) snake_case(StringType&& text)
    {
        return snake_case(std::forward<StringType>(text), '_', defaults::is_space, defaults::to_lower);
    }

    template<typename StringType, typename CharT>
    inline decltype(auto) snake_case(StringType&& text, CharT underscore)
    {
        return snake_case(std::forward<StringType>(text), underscore, defaults::is_space, defaults::to_lower);
    }

    template<typename StringType, typename CharT, typename IsWhitespace>
    inline decltype(auto) snake_case(StringType&& text, CharT underscore, IsWhitespace is_whitespace)
    {
        return snake_case(std::forward<StringType>(text), underscore, is_whitespace, defaults::to_lower);
    }
}

#endif
\$\endgroup\$
7
\$\begingroup\$

Case.h

I suggest naming C++ headers with .hpp or .hh or something else recognized as a C++ header extension. .h "works", but when I see a .h file, I assume it's a C header, or at the very least, a header designed to work with both C and C++. Code editors and syntax highlighters may make the same assumption.

I would also suggest that since you're doing more generic transforms, perhaps Case isn't the right name - perhaps TextTransform? Or something like that.

#include <utility>
#include <cctype>
#include <array>

I couldn't see any reason why <array> is included. Did you include it just to get std::begin()/std::end()? If so, the header you actually want is <iterator>.

It's also generally a good idea to order your includes in a predictable way, like alphabetical order. The reason is because #include is just a simple textual include - the contents of whatever file you're including are literally expanded in place. That means stuff from a previous include can affect stuff in a subsequent include - yes, that sucks, but that's why the C++ committee is working so hard toward modules to make #include obsolete. But what matters here is that since stuff in an include can be affected by what was previously included, including header files in a different order may change the interpretation. That won't actually happen for the standard includes (if it does, that's a serious bug!) but because varying the order of includes theoretically changes the result, having differently-ordered includes makes it harder for the compiler to precompute and cache includes, which can slow down compilation times.

template <typename Container, typename ConstIterator>
inline typename Container::iterator remove_constness(Container& c, ConstIterator it)
{
    return c.erase(it, it);
}

When I saw this function's name, I damn near had a heart-attack - removing constness is an incredibly dangerous thing to do! But then I saw what the function actually does, and it's not so crazy. Perhaps it would be better to give this function a more honest name, like to_non_const_iterator.

Since the entire purpose of the function is to transform a const_iterator to iterator, it doesn't seem worthwhile to take the argument as anything. Rather, the signature should probably be: template <Container> Container::iterator (Container&, Container::const_iterator). (For more generality, you could even add an overload template <Container> Container::iterator (Container&, Container::iterator) that just returns the argument unchanged.)

But a better idea would be to stop and think about whether any of this is a good idea in any case. Removing constness is an extremely dangerous thing to do, generally. The way you use it in transform_text() is safe... but the fact that you have to use it should be setting off alarm bells. You need this function because you want to use the same iterators returned from CasePolicy::find_next_word() in CasePolicy::transform_word(). Is that a good idea generally? Isn't there a better way to handle that?

An easy way out of that pickle would simply be to have CasePolicy::transform_word() not do its thing in-place. That is, instead of CasePolicy::transform_word() being defined like:

void transform_word(iterator begin_word, iterator end_word, StringType& text);

have it defined like:

void transform_word(const_iterator begin_word, const_iterator end_word, StringType& text, OutputIterator out);

Granted, that will be hard to make work with the runtime polymorphism you've opted to use for CasePolicy. But at the very least you could do:

void transform_word(const_iterator begin_word, const_iterator end_word, StringType& text, iterator out);

and specify that only [out, out + std::distance(begin_word, end_word)) gets written to (similar to std::copy(), for example - or more relevantly, std::transform()).

Also, a word about inline. You don't really need to mark template functions inline. It doesn't really have any effect. Modern compiler ignore it. inline is necessary for defining functions or variables that should only appear once in the program, but templates already work like that, so inline is superfluous.

const auto to_upper = [](int c)->char { return std::toupper(static_cast<unsigned char>(c)); };

This is the lone exception in your defaults functions that takes an int rather than a char. Typo?

template<typename StringType>
class CasePolicy {
public:
    using string_type = StringType;
    using iterator = typename StringType::iterator;
    using const_iterator = typename StringType::const_iterator;

    virtual std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) = 0;
    virtual void transform_word(iterator begin_word, iterator end_word, StringType& text) = 0;
    virtual bool meets_pred(const_iterator begin_word, const_iterator end_word) = 0;
};

So you've opted to go with runtime polymorphism for your policies. Cool. But if you're going to make a runtime polymophic class, you pretty much always want to define a virtual destructor. It just needs to be:

virtual ~CasePolicy() = default;

and you don't need to do anything in the derived classes (their destructors will automatically be virtual if the base class destructor is). Making the destructor virtual is essentially free if you already have other virtual functions, and it will prevent nasty, hard-to-diagnose bugs.

As for the interface you've chosen for your policy class, I know it's basically what I tossed out when brainstorming the idea, but when you stop to think about it... are all these functions really necessary in the base class interface?

The base case for transforming a text string seems pretty basic - in fact, it's pretty much just std::transform(), right? You could literally do an upper case transformation using std::transform() and your to_upper() function like this:

template <typename String>
auto uppercase(String text)
{
    using std::begin;
    using std::end;

    std::transform(begin(text), end(text), begin(text), to_upper);

    return text;
}

There's no need for the three-step dance of finding a word, checking the word, and then transforming the word. That process is necessary for the title-casing case, so it would make sense to have those functions in the title case policy... but not in the base policy.

You can even see how in UpperCasePolicy, your "find words" function just returns the whole string, and your predicate is just return true. They're just extra boilerplate you have to have deal with that are dead weight in most policies.

You could deal with that by providing them with default implementations in the base class that most policies would inherit and just use as is, and some (like title-casing) could override.

But you could also rethink how your transform function works. It seems to me like what you're doing is transforming the input by chunks, where those chunks are defined by (for example, when title-casing) whitespace vs non-whitespace (and possibly punctuation as well). The fact that you're transforming by chunks and not by element is why you can't simply use std::transform() - you need a class of some type to keep track of chunks and state. (An example of state would be title-case detecting when it's dealing with the first chunk, in which case it should uppercase the first letter even if it's an exception.)

If you think of it in terms of transforming chunks, your base policy could be as simple as:

template <typename InputIterator, typename OutputIterator>
struct TextTransformPolicyBase
{
    virtual ~TextTransformPolicyBase() = default;

    virtual auto transform_chunk(InputIterator first, InputIterator last, OutputIterator out) -> std::tuple<OutputIterator, InputIterator> = 0;
}

and the actual generic transform function could be basically:

template <typename String>
auto transform_text(String const& in, TextTransformPolicyBase<String::const_iterator, std::back_insert_iterator<String>>* policy)
{
    String result;
    result.reserve(in.size());
    auto out = std::back_inserter(result);

    for (auto it = in.cbegin(); it != in.cend(); )
        std::tie(out, it) = policy->transform_chunk(it, in.cend(), out);

    return result;
}

Uppercasing would be basically a one-liner:

template <typename InputIterator, typename OutputIterator>
struct UppercaseTransformPolicy : TextTransformPolicyBase<InputIterator, OutputIterator>
{
    auto transform_chunk(InputIterator first, InputIterator last, OutputIterator out) -> std::tuple<OutputIterator, InputIterator> override
    {
        return { std::transform(first, last, out, to_upper), last };
    }
}

Title casing would require additional member functions (like find_next_word() and meets_pred()) and data members to keep track of whether it's the first chunk of not, the exceptions list, and so on.

But again, that's all just off the top of my head; I really haven't put enough thought into it to be sure that it's a sound design. The point is:

  1. you don't need to over-complicate the base case, because you can add complication to dervied cases while leaving the base case simple; and
  2. you shouldn't need dangerous conversions between iterator types - that's a sign that your interface isn't correct.

I want to call attention to this function:

virtual void transform_word(iterator begin_word, iterator end_word, StringType& text) = 0;

You pass iterators to the result string... and also the result string itself. That seems unnecessary in the first place, but then it turns out that you actually manipulate the result string in this function in some policies... and then continue to use the iterators. That is extremely dangerous, because it would be very easy to invalidate the iterators while manipulating the string. In fact, you actually use erase() on the string in the snake case policy... that might invalidate iterators, throwing the whole transform_text() algorithm into UB-land.

template<typename StringType, typename PolicyString>
decltype(auto) transform_text(StringType&& text, CasePolicy<PolicyString>* policy)
{
    using std::begin;
    using std::end;

    PolicyString text_copy{ std::forward<StringType>(text) };

    auto first_word = policy->find_next_word(text_copy, begin(text_copy));

    auto begin_word = helper::remove_constness(text_copy, first_word.first);
    auto end_word = helper::remove_constness(text_copy, first_word.second);

    if (end_word == end(text_copy)) //check if there's only one word in string or string is empty
    {
        if (begin_word == end_word) return text_copy; //empty
        if (policy->meets_pred(begin_word, end_word)) {
            policy->transform_word(begin_word, end_word, text_copy);
        }
        return text_copy;
    }

    while (begin_word != end_word) // no more whitespace delimited words in the string
    {
        if (policy->meets_pred(begin_word, end_word)) {
            policy->transform_word(begin_word, end_word, text_copy);
        }

        if (end_word == end(text_copy)) break; // no more characters to observe in the string
        auto next_word = policy->find_next_word(text_copy, end_word + 1);

        begin_word = helper::remove_constness(text_copy, next_word.first);
        end_word = helper::remove_constness(text_copy, next_word.second);
    }
    return text_copy;
}

Now this is the meat of the entire library. So let's go through it from the top.

template<typename StringType, typename PolicyString>

Is there really any need for two different string types in the template parameter list? Especially considering that you're just copying the source string to the result as the first act of the function.

I suspect you chose to do it this way because you were running into problems with constness - for example, when StringType was a std::string const&, you couldn't use that for text_copy, and probably not for the type in your policy type. That makes sense but there is a simple way around it: using auto and std::decay.

For example, instead of:

template <typename StringType, typename PolicyString>
auto transform_text(StringType&& text, CasePolicy<PolicyString>* policy)
{
    // ...

    PolicyString text_copy{ std::forward<StringType>(text) };

    // ...

you could do:

template <typename StringType>
auto transform_text(StringType&& text, CasePolicy<std::decay_t<StringType>>* policy)
{
    // ...

    auto text_copy = std::forward<StringType>(text);

    // ...

Now, before you get into the main loop of your algorithm, you do a lot of preamble, and I'm not clear why. It looks like you've just essentially duplicated the guts of the loop. So why not refactor all that to be the first instance of the loop. For example:

for (auto begin_word = begin(text_copy); begin_word != end(text_copy);)
{
    auto [ begin_word, end_word ] = policy->find_next_word(text_copy, begin_word);

    if (policy->meets_pred(begin_word, end_word))
        policy->transform_word(begin_word, end_word, text_copy);

    begin_word = end_word;
}

That's essentially all you need, right? (Other than that in your current design, you need to deal with the conversion between const_iterator and iterator.) If the input is an empty string, the loop won't even run once... but if it contains a single word, then on the first run, begin_word will be begin(text_copy) and end_word will be end(text_copy)... meets_pred() will either return true or false - doesn't really matter, because if it returns true, transform_word() will transform an empty word - and then begin_word gets set to end_word, which is end(text_copy), and the loop ends.

auto next_word = policy->find_next_word(text_copy, end_word + 1);

This line concerns me. You check just before that end_word isn't end(text_copy), so at least you're not triggering undefined behaviour. But if begin_word is an iterator to the start of a word and end_word is one-past-the-end... which means end_word is the start of the next chunk... then aren't you skipping a character? Shouldn't finding the next work start immediately after the last word (even if that only means you'll probably be skipping spaces)?

I suspect the only reason it doesn't appear to be a problem is because you're doing the transform in place, and your policies all do some mucking around with the result string (because you pass it by reference) that hides the problem.

On with the review!

UpperCase.h

I'm going to go a bit out of order and start with upper_case(), since that is the simplest of the 3 transforms.

UpperCasePolicy(ToUpper to_upper) : m_to_upper{ to_upper } {}

You probably want to do a move here, to move to_upper into m_to_upper.

template<typename StringType, typename ToUpper = decltype(defaults::to_upper)>
inline auto make_upper_case_policy(ToUpper to_upper = defaults::to_upper) ->std::unique_ptr<UpperCasePolicy<StringType, ToUpper>>
{
    using UPC = UpperCasePolicy<StringType, ToUpper>;
    return std::make_unique<UPC>(to_upper);
}

template<typename StringType, typename ToUpper>
inline decltype(auto) upper_case(StringType&& text, ToUpper to_upper)
{
    using string_type = typename std::decay_t<StringType>;
    static auto policy = make_upper_case_policy<string_type>(to_upper);
    return transform_text(std::forward<StringType>(text), policy.get());
}

So what you're doing here is dynamically allocating your policy class, storing it in a static variable, and then passing the address of that to your generic function. There are a lot of problems with this.

First there is the simple matter of efficiency. There's no reason you need dynamic allocation for this. Your upper_case() function could just be:

template <typename StringType, typename ToUpper>
inline decltype(auto) upper_case(StringType&& text, ToUpper to_upper)
{
    static auto policy = UpperCasePolicy<std::decay_t<StringType>, ToUpper>{to_upper};
    return transform_text(std::forward<StringType>(text), &policy);
}

And then you don't need make_upper_case_policy() at all.

Now since you're already taking ToUpper as a template parameter, you might as well take advantage of perfect forwarding for it:

template <typename StringType, typename ToUpper>
inline decltype(auto) upper_case(StringType&& text, ToUpper&& to_upper)
{
    static auto policy = UpperCasePolicy<std::decay_t<StringType>, std::decay_t<ToUpper>>{std::forward<ToUpper>(to_upper)};
    return transform_text(std::forward<StringType>(text), &policy);
}

I mentioned above that inline is superfluous.

Now, for this and several functions you use decltype(auto) as the return type. Why? There doesn't seem to be any reason for doing so the way the code is currently written. Are you considering the possibility of an identity transform that just returns the same argument?

decltype(auto) is not something you should be using "just because". It is a powerful tool, but also one that easily leads to trouble. For example, some people like to put parentheses around their return statements (like return(result); instead of return result;). That's no problem for an auto return type, but it changes the meaning of a decltype(auto) return type, and possibly breaks things (by returning a reference to a local variable).

The bottom line is this: Don't use decltype(auto) as a return type unless you know you need it. If you're not sure, don't use it. It's at least unnecessary, unwise, and confusing, and it's probably wrong.

using string_type = typename std::decay_t<StringType>;

You don't need typename with decay_t. It's either typename std::decay<StringType>::type or std::decay_t<StringTyoe>.

static auto policy = make_upper_case_policy<string_type>(to_upper);

Why is policy static? I don't think that's what you want. This will only work if to_upper is something globally (statically) valid, like a pointer to a regular function, or a non-capturing lambda (and even that's dodgy), and only if there's nothing dynamic like allocation involved. And even then, it means that whatever function you call it with the first time... that's the function you'll be using for the rest of the program. Static variables initialize only once, not every time the function is called.

Put altogether, this is what you get:

template <typename StringType, typename ToUpper>
auto upper_case(StringType&& text, ToUpper&& to_upper)
{
    auto policy = UpperCasePolicy<std::decay_t<StringType>, std::decay_t<ToUpper>>{std::forward<ToUpper>(to_upper)};
    return transform_text(std::forward<StringType>(text), &policy);
}

The same applies for the overload:

template <typename StringType>
auto upper_case(StringType&& text)
{
    return upper_case(std::forward<StringType>(text), defaults::to_upper);
}

But back to the previous overload, I should point out that I can do this:

auto text = case_utils::upper_case("Text"s, case_utils::defaults::to_lower);

In fact, your UpperCasePolicy isn't really an uppercase policy at all. It's really a generic policy. In fact you could rename UpperCasePolicy to SimpleCasePolicy, and do this:

template <typename StringType, typename Transform>
class SimpleCasePolicy : public CasePolicy<StringType>
{
    Transform m_transform;

    // ... rest of the class is unchanged except for using
    // Transform and m_transform instead of
    // ToUpper and m_to_upper
};

template <typename String>
class UpperCasePolicy : SimpleCasePolicy<String, decltype(defaults::to_upper)>
{
    UpperCasePolicy() :
        SimpleCasePolicy<String, decltype(defaults::to_upper)>{defaults::to_upper}
    {}
};

template <typename String>
class LowerCasePolicy : SimpleCasePolicy<String, decltype(defaults::to_lower)>
{
    LowerCasePolicy() :
        SimpleCasePolicy<String, decltype(defaults::to_upper)>{defaults::to_lower}
    {}
};

// and more, like maybe Rot13Policy, and so on

SnakeCase.h

Technically, snake case can be implemented with a simple transform function and something like the SimpleCasePolicy above, because snake case can be done character by character without state. It's basically:

auto to_snake_case(char c)
{
    if (c == ' ')
        return '_';
    return std::to_lower(static_cast<unsigned char>(c));
}

template <typename String>
class SnakeCasePolicy : SimpleCasePolicy<String, char (*)(char)>
{
    SnakeCasePolicy() :
        SimpleCasePolicy<String, char (*)(char)>{to_snake_case}
    {}
};

However, your snake case collapses whitespace. There's nothing wrong with that, but it means that this is the first place in the review where we're dealing with a transform that produces a different amount of characters than the input. In this case, the number of characters in the output is strictly less than or equal to the number of input characters, so this is similar to standard library functions like copy_if().

    std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
    {
        using std::begin; using std::end;
        const_iterator begin_word = std::find_if_not(begin(text), end(text), m_is_whitespace);
        const_iterator end_word = std::find_if(begin_word, end(text), m_is_whitespace);
        return { begin_word, end_word };
    }

    void transform_word(iterator begin_word, iterator end_word, StringType& text) override
    {
        while (begin_word != end_word)
        {
            *begin_word = m_to_lower(*begin_word);
            ++begin_word;
        }
        if (end_word != end(text)) 
        {
            transform_whitespace(end_word, text);
        }
    }

    void transform_whitespace(iterator end_word, StringType& text)
    {
        using std::end;
        *end_word = m_underscore;
        ++end_word; //beginning of whitespace
        auto end_whitespace = std::find_if_not(end_word, end(text), m_is_whitespace);
        text.erase(end_word, end_whitespace);
    }

So the way this works is by skipping initial whitespace, and then from then on, lowercasing any non-whitespace, and collapsing all whitespace from then on into a single underscore for each whitespace run. So

  • "text" -> "text"
  • "TEXT" -> "text"
  • "This is the Text" -> "this_is_the_text"
  • "{space}{space}{space}foo{space}{space}{space}bar{space}{space}{space}" -> "foo_bar_"

This is where you first run into problems with your policy API. You want your transform function to modify the string in-place, but you also want to change the length of the sequence. You can't have it both ways. No standard algorithms do that; the closest are things like std::remove()/std::remove_if() which shuffles the sequence around and then lets you know where the new sequence bounds you should use are - you're responsible for resizing the sequence yourself (which is why you usually have to combine std::remove()/std::remove_if() with erase()).

Your options are basically:

  • Keep the in-place transform, but give up the ability to change the size of the result. At most you can support shortening the string, but not lengthening it.
  • Ditch in-place transform.

If you really want to keep in-place transforms, then you can still handle snake case the way you want, but you have to change the API. transform_word() should return an iterator to where it wrote the last output value, and then in your transform_text() function, you'd have to remove everything between the last output position and the next input position... but not with erase(), because that might invalidate the iterators.

Here's a concrete example of what I mean:

  1. The input is "foo bar" (that's "foo", then "bar" with 3 spaces between.
  2. "foo" is handled normally by the first part of transform_word().
  3. transform_whitespace() replaces the first space with an underscore. Then it returns a tuple of end_word and end_whitespace (and doesn't call text.erase(end_word, end_whitespace);, because that will cause undefined behaviour).
  4. So now the text is "foo_{space}{space}bar", where end_word points to the first space, and end_whitespace points to the "b". All of that information should be returned from transform_word().
  5. Back in transform_text(), you should do text_end = std::move(end_whitespace, text_end, end_word);. This will move everything from the "b" to the end of the string back over everything starting with the first space. So "foo_{space}{space}bar" becomes "foo_bar??" where "??" can be any two characters (will probably be "ar").
  6. end_whitespace is the new begin iterator for text, and text_end becomes the new end iterator for text. (Which means at the next iteration, you'll be starting from the "b" in "bar", and the end is one-past the "r".)

As you can see, this will require returning some iterators from transform_word() - one for the next output position and one for the next input position - so the API has to change.

All of this is needlessly complex, and still not all that flexible, so I'd recommend just giving up on in-place transformation, and writing the algorithms the same way as all the algorithms in <algorithm>: a pair of in iterators and an out iterator as input, then return the modified in and out iterators.

Here is what a snake case function might look like - and I don't intend to suggest this for your library, because it's a standalone thing, and won't integrate well with what you have; it's just to illustrate how powerful iterators are when you use them correctly:

// The most generic template.
template <typename InputIterator, typename Sentinel, typename OutputIterator, typename ToLower, typename IsSpace, typename CharT>
auto snake_case(InputIterator first, Sentinel last, OutputIterator out, ToLower&& to_lower, IsSpace&& is_space, CharT underscore)
{
    // Skip initial spaces.
    first = std::find_if_not(first, last, is_space);

    while (first != last)
    {
        // Lowercase word. (Too bad there's no std::transform_while().)
        while (first != last && !is_space(*first))
            *out++ = to_lower(*first++);

        // If we're not at the end, it means we hit a space.
        if (first != last)
        {
            // Write the underscore, then skip spaces. (Note that we
            // can use "++first" because know "first != last" and
            // "is_space(*first)" already.
            *out++ = underscore;
            first = std::find_if_not(++first, last, is_space);
        }
    }

    return out;
}

// The "normal" iterator template, which uses the locale.
template <typename InputIterator, typename Sentinel, typename OutputIterator>
auto snake_case(InputIterator first, Sentinel last, OutputIterator out, std::locale const& loc = {})
{
    auto const& ctype = std::use_facet<std::ctype<char>>(loc);

    auto const underscore = ctype.widen('_');

    auto const is_whitespace = [&loc](auto c) { return std::isspace(c, loc); };
    auto const to_lower = [&ctype](auto c) { return ctype.tolower(c); };

    return snake_case(first, last, out, to_lower, is_whitespace, underscore);
}

// Range template, for convenience.
template <typename InputRange, typename OutputIterator>
auto snake_case(InputRange range, OutputIterator out, std::locale const& loc = {})
{
    using std::begin;
    using std::end;

    return snake_case(begin(range), end(range), out, loc);
}

// Optimized string template.
template <typename CharT, typename Traits, typename Allocator>
auto snake_case(std::basic_string<CharT, Traits, Allocator> text)
{
    // Snake case promises that it will always output text.size()
    // characters or less. If it were more, we couldn't do this.
    auto const p = snake_case(text, text.begin());
    text.erase(p, text.end());
    return text;
}

// Usage examples:

auto s = snake_case("Something   to   snake   case"); // s is a string

auto const text = "input text read from file or something"s;
auto buffer = std::vector<char>{};
snake_case(text, std::back_inserter(buffer));

// Direct from input to output
snake_case(std::istreambuf_iterator<char>{std::cin}, std::istreambuf_iterator<char>{}, std::ostreambuf_iterator<char>{std::cout});

// And so on.

Note that that all this works even when the size of the output is less than the size of the input. But here's the insane part: almost all of it will also work if the output is larger than the input. The only function you'd need to change is the last one; all the examples still work.

Whether you choose to stick with in-place transforms, or change to using an out iterator model, you'll have to redesign the API to accommodate either way.

Moving on!

template<typename StringType, typename CharT>
inline decltype(auto) snake_case(StringType&& text, CharT underscore)
{
    return snake_case(std::forward<StringType>(text), underscore, defaults::is_space, defaults::to_lower);
}

template<typename StringType, typename CharT, typename IsWhitespace>
inline decltype(auto) snake_case(StringType&& text, CharT underscore, IsWhitespace is_whitespace)
{
    return snake_case(std::forward<StringType>(text), underscore, is_whitespace, defaults::to_lower);
}

While I generally appreciate templating as much as possible, some of this seems a bit superfluous. Since you're using defaults::to_lower at least, underscore can't really be anything but a char, can it?

Also, in the second function, is_whitespace could be perfectly-forwarded.

TitleCase.h

Finally to title-casing, the most complex of the three implementations you have (which is why Boccara chose it). (Simple/ASCII-only) upper-casing is just a basic run through std::transform(), snake-casing is stateless but produces output of a different length than the input, and finally there's title-casing, which is stateful (and if you allow more than simply/ASCII-only casing, can produce different length output). This is why you need a class (as opposed to the snake-casing code I wrote above, which because snake-casing is stateless, required no classes).

But one little thing before the class....

namespace defaults {
    using sv = std::string_view;
    constexpr auto title_case_exceptions_list = std::array{ sv{ "a" }, sv{ "an" }, sv{ "and" }, sv{ "as" }, sv{ "at" }, sv{ "by" }, sv{ "for" }, sv{ "in" }, sv{ "of" }, sv{ "on" }, sv{ "to" }, sv{ "or" }, sv{ "the" } };
}

If you're going to be doing using sv = std::string_view; in the defaults namespace anyway, you might as well use string view literals:

namespace defaults {
    using namespace std::literals::string_literals;
    constexpr auto title_case_exceptions_list = std::array{ "a"sv, "an"sv, // and so on
}

That should save a lot of typing.

So let's get into the class....

To start with, I don't see any sort of flag in the class's data members to keep track of whether it's done with the first word or not. Is that special case handled?

//find first whitespace-delimited word starting from position first
std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
{
    using std::end;

    const_iterator begin_word = std::find_if_not(first, end(text), m_is_whitespace);
    const_iterator end_word = std::find_if(begin_word, end(text), m_is_whitespace);
    return { begin_word, end_word };
}

Here is the first sign of trouble with your API. This function returns an iterator pair to the first "word" (for whatever title-casing considers a word). Okay, but to get it, you skip over a bunch of whitespace. Now, here's the question: what happens to everything between first and begin_word?

Let me give a concrete example. Let's say the input text is "{space}{space}{space}foo bar" - three spaces and then "foo", space, "bar". In transform_text(), the first step is to call find_next_word() using the policy class. That will return a pair of iterators pointing to "f" and the space between "foo" and "bar". But now what does transform_text() do with the first three spaces? Does it just copy them to the output? Discard them?

You're designing your policy class API around a single use-case: title-casing. You're not thinking generally. Think of all the different kinds of transforms someone might be interested in:

  • Stripping all spaces (but otherwise leaving text intact).
  • Replacing all whitespace runs with a single space (such as needed for HTML viewing), and tokenizing C++ (ignoring literals).
  • Replacing all whitespace with visible characters (for proofreading).
  • Escaping/unescaping strings (like converting newlines to "\n", or the other way around).

Spaces are handled differently in all of those cases.

As currently written, text_transform() ignores whitespace. In order to get snake-casing to work - an algorithm that cares about whitespace - you have to mangle the source string.

There's another issue, too. The way text_transform() and the policy API works, for each chunk you have to first call find_next_word(), then throw all the information you got during that function, then test the predicate, then do the transform. That's not particularly efficient. And for most transforms, those three steps aren't even required. Granted, title-casing requires multiple passes over each chunk - after you've figured out the word, you then need to test it against the exceptions list and that it's not an acronym, and only if it doesn't match do you do any actual transformations (maybe).

Now, there is no "right" way to deal with all this. This challenge is all about figuring out the engineering trade-offs you have to make.

Here's one possible solution: First, I would say forget run-time polymorphism. Run-time polymorphism generally sucks - it's complicated, it's error-prone, it's restricting, and it's slow. You're using templates anyway, so you can do everything with compile-time polymorphism.

Once you move to compile-time polymorphism, you're no longer restricted in what types you can use. So perhaps your policy class could have a function that returns an iterator to the end of the next chunk, and some kind of type with the information about that chunk.

For example, let's consider snake case (because it's more complex than upper-casing, but less than title-casing). The policy class for snake-casing might look like:

class SnakeCasePolicy
{
    // chunk_type is part of the policy API, as you'll see below
    struct chunk_type
    {
        // The only information we need about a snake-case chunk
        // is whether it's whitespace or not.
        bool is_whitespace;
    };

    template <typename ForwardIterator>
    auto get_next_chunk(ForwardIterator first, ForwardIterator last) ->
        std::tuple<ForwardIterator, chunk_type>
    {
        if (first != end && is_space(*first))
            return { std::find_if_not(first, last, is_space), chunk_type{true} };
        return { std::find_if(first, last, is_space), chunk_type{false} };
    }

    template <typename ForwardIterator, typename OutputIterator>
    auto transform_chunk(ForwardIterator first, ForwardIterator last, OutputIterator out, chunk_type const& ct) ->
        OutputIterator
    {
        if (ct.is_whitespace)
            *out++ = underscore;
        else
            out = std::transform(first, last, out, to_lower);

        return out;
    }
};

Obviously the class will need to be templated with members for is_space() and to_lower() and underscore.

And text_transform() might look like this:

template <typename ForwardIterator, typename OutputIterator, typename Policy>
auto text_transform(ForwardIterator first, ForwardIterator last, OutputIterator out, Policy&& policy)
{
    while (first != last)
    {
        // Get the next chunk of text (which will be [first, next),
        // along with info about the chunk.
        auto const [ next, info ] = policy.get_next_chunk(first, last);

        // Transform the chunk, using the info we already got about it.
        out = policy.transform_chunk(first, next, out, info);

        // Move the start of the input to the end of what's already
        // been processed.
        first = next;
    }

    return out;
}

template <typename ForwardIterator, typename OutputIterator, typename Policy>
auto snake_case(ForwardIterator first, ForwardIterator last, OutputIterator out)
{
    return text_transform(first, last, out, SnakeCasePolicy{});
}

// You could add more overloads for policy parameters, or simply let
// users do:
text_transform(f, l, o, SnakeCasePolicy{my_to_lower_func, my_is_space_func, my_underscore});

Upper-casing is trivial, too:

class UpperCasePolicy
{
    // Don't need chunk type info, so just...
    using chunk_type = void*; // or int or something

    template <typename ForwardIterator>
    constexpr auto get_next_chunk(ForwardIterator first, ForwardIterator last) noexcept ->
        std::tuple<ForwardIterator, chunk_type>
    {
        // No need to break into chunks, though you could.
        return { last, nullptr };
    }

    // This could also be constexpr as of C++20.
    template <typename ForwardIterator, typename OutputIterator>
    auto transform_chunk(ForwardIterator first, ForwardIterator last, OutputIterator out, chunk_type)
    {
        return std::transform(first, last, out, to_upper);
    }
};

Or as mentioned before, you could have a generic base that uses different functions in the std::transform() call. The generic base would basically be what's above, and then UpperCasePolicy will derive from GenericCasePolicy<ToUpper>, and the entire class body will just be a using directive for the constructor that takes the function object.

Note that using compile-time polymorphism allows flexibility in the types and calling conventions. We can declare functions noexcept or constexpr and get benefits from that. We can use different types for the chunk_type, and include as much information as you want. Also, rather than returning a chunk_type and the iterator, you might instead return a chunk struct that has all the info about the chunk including the iterator range. So the signature of transform_chunk could just be std::tuple<OutputIterator, InputIterator> transform_chunk(chunk, OutputIterator);, and transform_text() might be as simple as this:

template <typename ForwardIterator, typename OutputIterator, typename Policy>
auto text_transform(ForwardIterator first, ForwardIterator last, OutputIterator out, Policy&& policy)
{
    while (first != last)
        std::tie(out, first) = policy.transform_chunk(policy.get_next_chunk(first, last), out);

    return out;
}

For title-casing, the chunk_type (or chunk) would not only record if the chunk type is whitespace or not. It could also note whether it was all caps, or in the exception list. That would put all the "checking" logic in get_next_chunk(), and all the "transforming" logic in transform_chunk().

And you'd probably want a base class for title-casing, then derived classes for title-casing with and without handling acronyms. None of that requires run-time polymorphism or virtual functions.

But of course, that's just one possibility for the API design.

The rest of the title-casing stuff has pretty much already been covered while talking about the other algorithms.

Summary

You have one critical problem to fix, and a couple fairly serious ones, that I spotted (where "critical" means definitely UB, and "serious" means maybe UB if not used carefully).

The critical problem is that you are using string iterators while modifying the string itself at the same time. That's a big no-no. String iterators are crazy fickle; they can invalidate if you do basically anything to the string other than calling its const functions, and a handful of others (like operator[], begin, and so on). (Note that this is only the case for std::string. Other containers' iterators aren't quite so fickle.)

The serious problems are:

  • If you're going to be using run-time polymorphism, you should really have a virtual destructor in the base class. Without that, it's too easy to end up with partially-destructed objects.
  • Don't use decltype(auto) unless you're damn sure you need it, because it's very finicky, and all too easy to trigger UB with.

As for bugs - things that aren't UB, but probably won't work the way you expect:

  • You use function static variables for the policies in your "make policy" functions. I don't think that's what you want. It means that only the first policy your functions ever get called with will be used. (And you might trigger UB if the lifetime of that one policy that gets set isn't as long as the entire program.)
  • You've got a very suspicious incrementing of an iterator in text_transform() (on the line where you set next_word). I suspect the reason you haven't noticed any problems yet is because all of your algorithms involve words with stuff in between... spaces, to be precise. But if someone made a policy that broke text into words without characters between, they might notice weirdness. (For example, someone might make a policy to convert CamelCase to snake_case, where "Camel" is one word and "Case" is the next, with nothing between.)

You have a couple of design issues I'd look into fixing:

  • You should never have to wipe away constness, including converting const_iterators to iterators. The fact that you have to do that is a red flag that your API (or the way you're using it) needs rethinking.
  • Your algorthim and the policy API makes certain assumptions about what kind of transforms you're going to support - in particular, it's in terms of "words" without any general understanding of how things that are not words are to be handled. Currently, the algorithm more-or-less just skips over anything that's not a word (which is why you have to mangle the string to make snake-casing work). None of this is "wrong" - a words-based API is not "incorrect" in any sense - but maybe your API should also include some notion of what to do with the stuff between words: preserve it? remove it? transform it (as in the case of snake-casing)?
  • You've built your policy hierarchy with the intention of using run-time polymorphism for it... but run-time polymorphism is grody; it's slow, it's bloated, it's brittle, it's inflexible. Compile-time polymorphism is more flexible, and the compiler can very often optimize it to essentially nothing. It would be much easier for me as a user to make new policy classes without being shackled by override restrictions (for example, I could make functions constexpr). (But maybe a is_text_transform_policy traits type (or concept! C++20 has them now!) could be handy for testing.)
\$\endgroup\$
4
\$\begingroup\$
template<typename StringType, typename PolicyString>
decltype(auto) transform_text(StringType&& text, CasePolicy<PolicyString>* policy)

Why are you using decltype(auto)? You’re simply returning a PolicyString from a local variable, and I don’t see anything about value categories.


//converts const_iterator to iterator, needed to convert result of find_next_word() without changing 
//param to StringType& which contradicts the purpose of the function
template <typename Container, typename ConstIterator>
inline typename Container::iterator remove_constness(Container& c, ConstIterator it)
{
    return c.erase(it, it);
}

Good for commenting the purpose and need for the function.
But, that seems like a strange way to do it. The fact that I don’t know of a simple way to do this off hand points to how odd it is to need such a thing.

find_next_word should probably return a non-const iterator if given non-const input. Model it on std::find_if: It returns the same type as the iterator passed in, whether it is const or non.


namespace defaults {
    const auto to_upper = [](int c)->char { return std::toupper(static_cast<unsigned char>(c)); };
      ⋮ et al.

Good idea to wrap the troublesome legacy std functions. I wonder why the namespace is called defaults though; and why you wrote it as function objects rather than functions. I’ve used function objects to prevent overloading and searching for more overloads once that is found.


template<typename StringType, typename PolicyString>
decltype(auto) transform_text(StringType&& text, CasePolicy<PolicyString>* policy)

Just reading through, with no documentation or design diagrams, I’m confused at this point. I see CasePolicy is parameterized by how strings are represented. So what is a PolicyString? It seems to me that the policy’s type argument should be the same as the StringType called with, so that the text can in fact be passed to the policy’s functions.

    PolicyString text_copy{ std::forward<StringType>(text) };

OK, you recopy whatever was passed in, so it can be a different type. This is also what is being returned. I don’t think it is necessary to allow for different input and output types like this.

Since you are treating the input parameter as a sink param, it would be fine to make it the same type, and let any conversion take place at the caller’s side.

    auto first_word = policy->find_next_word(text_copy, begin(text_copy));

That’s odd: the find⋯ function takes the whole container and the begin iterator? As opposed to the begin and end, or a range holding the same?


I think the idea of having the generic logic in transform_text and separate out the specific rules in a policy class is a good idea. The transform_text is more complex then necessary, as explained above, but it is clear when reading through it and it has a single responsibility.

I wonder though about the Policy being a virtual base class. It still needs to be a template since you parameterized the policy class on the string type, and transform_text is itself a template and in the header file.

So you could have the specific concrete policy passed in as a template argument, and not need virtual function calls.


using sv = std::string_view;
constexpr auto title_case_exceptions_list = std::array{ sv{ "a" }, sv{ "an" }, sv{ "and" }, sv{ "as" }, sv{ "at" }, sv{ "by" }, sv{ "for" }, sv{ "in" }, sv{ "of" }, sv{ "on" }, sv{ "to" }, sv{ "or" }, sv{ "the" } };

How about

using namespace std::literals::string_literals;
⋯ "and"sv, "as"sv, "at"sv ⋯

bool is_exception(InputIterator begin_word, InputIterator end_word, const ForwardRange& exceptions)

You are implementing a “is any of” for a hard-coded collection. Just call std::any_of!

Something like:

using std::begin; using std::end;
const auto result = std::any_of(begin(exceptions), end(exceptions), 
    [=](auto exception){ return std::equal(std::equal(begin(exception), end(exception), begin_word, end_word); } );
return result != end(exceptions);

But for is_acronym you did use the library functions. Hmm.


Summary

I think the style and use of language features is well up to date, in keeping with the advice being posed in this S.E. You certainly get an A+ for taking in all of that!

I find the overall body of code to be rather complicated. There is a balance between flexibility and complexity, and this is probably overkill for a component of a specific application, where you will know the details of what you are doing with it. It is more like the kind of fit-for-everyone that you see in a library like some of the std and Boost code. Configuring a detail of something that’s used deep inside other code is very handy. But if that thing itself is accessible to the consumer of the library, you have to think about when it is easier to just replace that component than to configure it!

With that kind of flexibility, you will have your work cut out for you in testing!

As a lesson in writing that kind of policy-driven generic code (lesson code is always toy-ish and seemingly overkill if taken at face value rather than as a lesson), I’d give it a B+. Marks off for lack of design documentation/explaination, and (possibly? might make sense knowing the design and intended use case) the mixture of run-time polymorphism with templates in the policy class.

In any case, keep it up!

\$\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.