5
\$\begingroup\$

I created a string splitter in C++. It splits a string in a lazy fashion and makes use of forward iterators to sequentially supply the next token from the string until it runs out of tokens.

#include <string>
#include <algorithm>
#include <regex>
#include <istream>
#include <iostream>
#include <vector>
#include <utility>
#include <iterator>
#include <initializer_list>
#include <type_traits>
#include <unordered_set>

class StringSplitter {

    template <bool is_const = false>
    class SplitIterator: public std::iterator<std::forward_iterator_tag, 
        typename std::conditional<is_const, const std::string, std::string>::type> {

        // The type of the elements to be iterated over
        using Type = typename std::iterator_traits<SplitIterator>::value_type;
        using UnqualifiedType = typename std::remove_const<Type>::type;

        friend class StringSplitter;

        const StringSplitter& split;
        UnqualifiedType token;
        std::smatch results;
        typename Type::const_iterator pos, fin;

        /**
         * @brief This constructor takes a StringSplitter reference to make use of
         * 
         * @param splitter The underlying StringSplitter object which
         * contains the string to split
         */
        SplitIterator (const StringSplitter& splitter) :split(splitter) {
            this->pos = splitter.string_to_split.begin();
            this->fin = splitter.string_to_split.end();
            this->prepare_next_token();
            std::cout << "Iterator String split object copy ctor called\n";
        }

        SplitIterator (const StringSplitter&& splitter) 
            :split(std::move(splitter)) {
                this->pos = split.string_to_split.begin();
                this->fin = split.string_to_split.end();
                this->prepare_next_token();
                std::cout << "Iterator String split object move ctor called\n";
            }

        /**
         * @brief Creates the next token which can be accessed by dereferencing
         * the iterator
         * @details If the iterator has run out of string to split, it will set
         * the token to the empty string
         * Otherwise, it will search for the next occurence of the delimiters supplied
         * and return the next token available
         */
        void prepare_next_token() noexcept {
            if (this->pos == this->fin) {
                this->token = "";
                return;
            }

            if (!std::regex_search(this->pos, this->fin, this->results, this->split.pattern,
                std::regex_constants::match_not_null)) {
                // If the end of the string is reached without finding a delimiter
                // then copy the rest of the string and set pos to fin
                if (this->results.suffix().length() == 0) {
                    this->token = std::string(pos, fin);
                    this->pos = this->fin;
                // otherwise, take the token before this delimiter
                } else this->token = this->results.prefix();

            } else {
                this->token = this->results.prefix();
                this->pos += this->results.position() + this->results.length();
            }
        }

        /**
         * @brief Used to determine if the iterator is still in a valid state
         * @details This does not guarantee that the iterator is valid
         * due to the possiblity of modifying token from outside the class
         * @return A boolean indicating if the iterator contains a non-empty token
         */
        bool is_valid() const {
            return !this->token.empty();
        }

        public:
            // Satisfy Default Constructible
            SplitIterator(): SplitIterator(std::move(StringSplitter(""))) {}

            SplitIterator (const SplitIterator& other)
                :split(other.split), token(other.token),
                results(other.results), pos(other.pos),
                fin(other.fin) {

                std::cout << "Iter copy ctor called\n";
            }

            SplitIterator (const SplitIterator&& other)
                :split(std::move(other.split)), token(std::move(other.token)),
                results(std::move(other.results)), pos(std::move(other.pos)),
                fin(std::move(other.fin)) {

                std::cout << "Iter move ctor called\n";
            }

            // Satisfy Swappable
            void swap(SplitIterator& other) noexcept {
                std::swap(this->split, other.split);
                std::swap(this->pos, other.pos);
                std::swap(this->fin, other.fin);
                std::swap(this->token, other.token);
                std::swap(this->results, other.results);
            }

            SplitIterator& operator++() { // Pre-increment
                std::cout << "Pre-increment called!!\n";
                this->prepare_next_token();
                return *this;
            }

            SplitIterator operator++(int) { // Post-increment
                std::cout << "Post-increment called!!\n";
                SplitIterator prev_i = *this;
                this->prepare_next_token();
                return prev_i;
            }

            // Satisfy EqualityComparable
            bool operator == (const SplitIterator& rhs) const {
                return this->token == rhs.token;
            }

            // Satisfy EqualityComparable
            bool operator != (const SplitIterator& rhs) const {
                return !(*this == rhs);
            }

            Type& operator*() {
                return token;
            }

            Type operator* () const {
                return token;
            }

            Type const* operator-> () const {
                return &token[0];
            }

            Type *operator->() {
                return &token[0];
            }           
    };

    const std::string string_to_split;
    std::regex pattern;

    StringSplitter(const std::string& str)
        :string_to_split(str) {
        this->pattern = std::regex("\\s+");
        std::cout << "StringSplitter Copy String ctor called\n";
    }

    StringSplitter(const std::string&& str)
        :string_to_split(std::move(str)) {
        this->pattern = std::regex("\\s+");
        std::cout << "StringSplitter Move String ctor called\n";
    }

public:

    StringSplitter(const StringSplitter& other)
        :string_to_split(other.string_to_split),
        pattern(other.pattern) {
            std::cout << "StringSplitter Copy ctor called\n";
        }

    StringSplitter(const StringSplitter &&other)
        :string_to_split(std::move(other.string_to_split)),
        pattern(std::move(other.pattern)) {
            std::cout << "StringSplitter Move ctor called\n";
        }

    using iterator = SplitIterator<false>;
    using const_iterator = SplitIterator<true>;

    /**
     * @brief Reads the string from a stream
     * @details The string in the stream is read and stored in the
     * StringSplitter object
     * 
     * @param stream A valid stream that is open for reading
     * @return A StringSplitter object
     */
    static StringSplitter with(std::istream& stream);

    /**
     * @brief The string to split is supplied as argument
     * @details Does not modify the original string therefore it is
     * passed as a constant reference
     * 
     * @param str The string to split
     * @return A StringSplitter object
     */
    static StringSplitter with(const std::string& str);

    /**
     * @brief Accepts an rvalue string
     * 
     * @param str A string to split on
     * @return A StringSplitter object
     */
    static StringSplitter with(const std::string&& str);

    /**
     * @brief Split the string by a regex pattern
     * @details The pattern supplied must be a valid regex but shouldn't
     * have any quantifiers associated with it
     * For example, to split on white space, the string should be
     * "\\s" not "\\s*" or "\\s+"
     * 
     * @param reg The string containing the pattern to split on
     * @return A String splitter
     */
    const StringSplitter by_pattern(const std::string&);

    /**
     * @brief Allows the splitting of the string through the use of characters
     * @details If you want to split instead by some characters, then you can
     * specify those characters as the parameters to this method
     * 
     * @param ch An array of characters
     * @return A String splitter
     */
    const StringSplitter by_any(const std::initializer_list<char> chars);

    /**
     * @brief Can be used to create a vector from the split string
     * @return A vector containing the split string
     */
    operator std::vector<std::string> () const;

    // Iterator stuff
    iterator begin();
    iterator begin() const;
    const_iterator cbegin();
    iterator end();
    iterator end() const;
    const_iterator cend();
};    

StringSplitter StringSplitter::with(std::istream& iss) {
    std::ostream* ptr = iss.tie(nullptr);

    // http://insanecoding.blogspot.ca/2011/11/how-to-read-in-file-in-c.html
    std::string content;
    iss.seekg(0, std::ios::end);
    content.resize(iss.tellg());
    iss.read(&content[0], content.size());

    iss.tie(ptr);

    return StringSplitter::with(content);
}

StringSplitter StringSplitter::with(const std::string& str) {
    return StringSplitter(str);
}

StringSplitter StringSplitter::with(const std::string&& str) {
    return StringSplitter(std::move(str));
}

const StringSplitter StringSplitter::by_any(const std::initializer_list<char> chars) {
    std::string reg = std::accumulate(chars.begin(), chars.end(), std::string{},
        [](std::string& str, const char c) {
            return str.empty() ? str + "[" + c : str + "|" + c;
        }) + "]";

    return StringSplitter::by_pattern(reg);
}

const StringSplitter StringSplitter::by_pattern(const std::string& str) {
    this->pattern = str + '*';
    return *this;
}

StringSplitter::operator std::vector<std::string> () const {
    return std::vector<std::string>{begin(), end()};
}

StringSplitter::iterator StringSplitter::begin() {
    std::cout << "Begin called!\n";
    return iterator(*this);
}

StringSplitter::iterator StringSplitter::begin() const {
    std::cout << "CV Begin called!\n";
    return iterator(*this);
}

StringSplitter::const_iterator StringSplitter::cbegin() {
    std::cout << "Const Begin called!\n";
    return const_iterator(*this);
}

StringSplitter::iterator StringSplitter::end() {
    std::cout << "End called!\n";
    return iterator();
}

StringSplitter::iterator StringSplitter::end() const {
    std::cout << "CV End called!\n";
    return iterator();
}

StringSplitter::const_iterator StringSplitter::cend() {
    std::cout << "Const End called!\n";
    return const_iterator();
}

I have some concerns with the code. Here is the main method that makes use of the class:

auto main() -> int {
    std::vector<std::string> tokens = 
    StringSplitter::with("Hello     my !; name is!Slim Shady!")
        .by_any({' ', '!', ';'});

    for (const auto& str : tokens) {
        std::cout << str << std::endl;
    }
    return 0;
}

The output is this:

StringSplitter Move String ctor called
StringSplitter Copy ctor called
CV Begin called!
Iterator String split object copy ctor called
CV End called!
StringSplitter Move String ctor called
Iterator String split object move ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Iter copy ctor called
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Pre-increment called!!
Hello
my
name
is
Slim
Shady

This output is my main concern with the code. I admit I went a bit wild the constructors, but it's been a while since I have played with OOP in C++, so the idea of rvalues, lvalues, gvalues, move ctors, copy ctors, etc was very confusing.

  • Why are there soo many calls to copying the iterator. The ++() operator for the iterator class returns a reference, so I don't understand why there is soo much copying going on
  • Also why does it seem to be doing everything twice? As you can see from the main method, it is only used once but the output looks like it was used twice.
  • Suggestions are welcome even if it means scrapping the current implementation.
\$\endgroup\$
  • \$\begingroup\$ Just a quick comment: const T&& is nonsense. You cannot move from a const object or reference, so in practice that is the same as a const ref. You should drop the const in those overloads. \$\endgroup\$ – glampert Apr 28 '16 at 18:55
2
\$\begingroup\$

I'm going to focus on your iterator first.

The way you've templated it is a little strange. Typically you'll just template it as a <T> and then give either std::string or const std::string as the type.

Also, you almost never see this in C++ code - using it means you're either shadowing a member variable with a parameter (don't do this) or it is completely redundant (don't do this either).

You should also prefer colon initialization as well whenever possible.

I don't like that you have all of these debug statements printing to stdout, and all the time. For this situation I'd probably use something like

#ifdef DEBUG_MODE
std::cerr << "Iterator String split object copy ctor called\n";
#endif

Even better would be to wrap it in a function.

void logMessage(const std::string& message, ostream& out = std::cerr) {
    #ifdef DEBUG_MODE
        out << message;
    #endif
}

I've never seen a move constructor where the parameter is const - I don't actually know what, if any impact that would have, but it is surprising and shouldn't have an impact on anything. I'm also not sure of a situation when you'd actually want the iterator to use move construction - in the interest of YAGNI, I'd say just remove it.

Your constructor is not ideal - I'd explicitly pass in begin, end, and pattern.

In prepare_next_token I think you don't need to check the case of it being at the end - in C++ we generally prefer to make the user handle those cases, and not introduce extra overhead for everyone else.

I don't see a point to is_valid, you don't use it anywhere I see.

A default constructor should be just that - default. I don't think it is worth setting the values to be anything (it might even make something work, or seem to work, that it shouldn't if you don't let them be default initialized).

Equality seems questionable to me - I think you'd also want to check that they have the same position within the string (it seems conceivable that a string could have the same token within it twice, although I don't know much about C++ regex).

It is also completely unnecessary to have a reference to your StringSplitter in the iterator (and is probably not what you actually want, too). Instead, you should store the pattern in your iterator as well, and then you can get rid of it entirely.

I also wouldn't put the iterator at the very top of your class - I generally prefer to put private things, including my iterators, at the end.

Also, your

begin()/end() const;

functions should return a const_iterator, not an iterator.

I'd rewrite your iterator to look something like this.

void logMessage(const std::string& message, ostream& out = std::cerr) {
    #ifdef DEBUG_MODE
        out << message;
    #endif
}

template <typename T>
class SplitIterator: public std::iterator<std::forward_iterator_tag, T> {

    using unqualified_type = typename std::remove_const<value_type>::type;
    using const_reference = const reference;
    using const_pointer = const pointer;

    friend class StringSplitter;

    unqualified_type token;
    std::smatch results;
    std::regex pattern;
    typename value_type::const_iterator pos, fin;

    SplitIterator(
        const std::string& pattern, typename value_type::const_iterator start,
        typename value_type::const_iterator end)
      : pattern(pattern), pos(start), fin(end) {
        prepare_next_token();
      }

    /**
     * @brief Creates the next token which can be accessed by dereferencing
     * the iterator
     * @details It will search for the next occurence of the delimiters supplied
     * and return the next token available
     */
    void prepare_next_token() noexcept {
        if (!std::regex_search(pos, fin, results, pattern, std::regex_constants::match_not_null)) {
            // If the end of the string is reached without finding a delimiter
            // then copy the rest of the string and set pos to fin
            if (!results.suffix().length()) {
                token = std::string(pos, fin);
                pos = fin;
            } else {
                token = results.prefix();
            }
        } else {
            token = results.prefix();
            pos += results.position() + results.length();
        }
    }

    public:
        // Satisfy Default Constructible
        SplitIterator() {}

        SplitIterator (const SplitIterator<T>& other)
            :pattern(other.pattern), token(other.token),
            results(other.results), pos(other.pos),
            fin(other.fin) {

            logMessage("Iter copy ctor called\n");
        }

        // Satisfy Swappable
        void swap(SplitIterator<T>& other) noexcept {
            std::swap(pattern, other.pattern);
            std::swap(pos, other.pos);
            std::swap(fin, other.fin);
            std::swap(token, other.token);
            std::swap(results, other.results);
        }

        SplitIterator<T>& operator++() {
            logMessage("Pre-increment called!!\n");
            prepare_next_token();
            return *this;
        }

        SplitIterator<T> operator++(int) {
            logMessage("Post-increment called!!\n");
            SplitIterator<T> prev_i = *this;
            prepare_next_token();
            return prev_i;
        }

        bool operator==(const SplitIterator<T>& rhs) const {
            return pos == rhs.pos;
        }

        // Satisfy EqualityComparable
        bool operator != (const SplitIterator<T>& rhs) const {
            return !operator==(rhs);
        }

        reference operator*() {
            return token;
        }

        const_reference operator*() const {
            return token;
        }

        pointer operator->() {
            return &token[0];
        }       

        const_pointer operator->() const {
            return &token[0];
        }    
};

Now onto your main class.

As before, don't use this-> and use some logMessage function (or similar).

I don't like that you're mixing your inline function definitions with declarations - imo pick one and stick with it.

You structure is a little odd - I'd expect all of the constructors to look something like this:

StringSplitter(some_type string_source, const std::string& pattern)

I also wouldn't expect any of them to be static.

I don't like that the normal constructors are private - why not just use those?

The by_pattern and by_any are a little weird - I'd prefer that you just use a setter (or again, a constructor...).

I don't like that std::vector is privileged - you provide an iterator, so people who want to construct their containers from this can do so themselves.

I changed end to explicitly pass in string_to_split.end() as the starting iterator so as to actually represent some end value. You might have to tweak the constructor or prepare_next_token so this doesn't do something unexpected.

I'd rewrite that class like so (I didn't include the iterator again).

class StringSplitter {

    static const std::string default_pattern = "\\s+";
    const std::string string_to_split;
    std::regex pattern;

public:

    StringSplitter(const std::string& str, const std::string& pattern = default_pattern)
      : string_to_split(str), pattern(pattern) {
        logMessage("StringSplitter Copy String ctor called\n");
    }

    StringSplitter(std::string&& str, const std::string& pattern = default_pattern)
      : string_to_split(std::move(str)), pattern(pattern) {
        logMessage("StringSplitter Move String ctor called\n");
    }

    StringSplitter(const StringSplitter& other)
      : string_to_split(other.string_to_split), pattern(other.pattern) {
        logMessage("StringSplitter Copy ctor called\n");
    }

    StringSplitter(StringSplitter &&other)
      : string_to_split(std::move(other.string_to_split)), pattern(std::move(other.pattern)) {
        logMessage("StringSplitter Move ctor called\n");
    }

    using iterator = SplitIterator<std::string>;
    using const_iterator = SplitIterator<const std::string>;

    /**
     * @brief Reads the string from a stream
     * @details The string in the stream is read and stored in the
     * StringSplitter object
     * 
     * @param stream A valid stream that is open for reading
     */
    StringSplitter(std::istream& streamm, const std::string& pattern = default_pattern);

    /**
     * @brief Split the string by a regex pattern
     * @details The pattern supplied must be a valid regex but shouldn't
     * have any quantifiers associated with it
     * For example, to split on white space, the string should be
     * "\\s" not "\\s*" or "\\s+"
     * 
     * @param reg The string containing the pattern to split on
     */
    void set_split_pattern(const std::string& pattern);

    /**
     * @brief Allows the splitting of the string through the use of characters
     * @details If you want to split instead by some characters, then you can
     * specify those characters as the parameters to this method
     * 
     * @param ch An array of characters
     */
    void set_split_characters((const std::initializer_list<char> chars);

    // Iterator stuff
    iterator begin();
    const_iterator begin() const;
    const_iterator cbegin();
    iterator end();
    const_iterator end() const;
    const_iterator cend();
};    

StringSplitter(std::istream& iss, const std::string& pattern) : pattern(pattern) {
    std::ostream* ptr = iss.tie(nullptr);

    // http://insanecoding.blogspot.ca/2011/11/how-to-read-in-file-in-c.html
    std::string content;
    iss.seekg(0, std::ios::end);
    content.resize(iss.tellg());
    iss.read(&content[0], content.size());

    iss.tie(ptr);

    string_to_split = content;
}

void StringSplitter::set_split_characters(const std::initializer_list<char> chars) {
    std::string reg = std::accumulate(chars.begin(), chars.end(), std::string{},
        [](std::string& str, const char c) {
            return str.empty() ? str + "[" + c : str + "|" + c;
        }) + "]";

    set_split_pattern(reg);
}

void StringSplitter::set_split_pattern(const std::string& str) {
    pattern = str + '*';
}

StringSplitter::iterator StringSplitter::begin() {
    logMessage("Begin called!\n");
    return iterator(pattern, string_to_split.begin(), string_to_split.end());
}

StringSplitter::const_iterator StringSplitter::begin() const {
    logMessage("CV Begin called!\n");
    return const_iterator(pattern, string_to_split.begin(), string_to_split.end());
}

StringSplitter::const_iterator StringSplitter::cbegin() {
    logMessage("Const Begin called!\n");
    return const_iterator(pattern, string_to_split.begin(), string_to_split.end());
}

StringSplitter::iterator StringSplitter::end() {
    logMessage("End called!\n");
    return iterator(pattern, string_to_split.end(), string_to_split.end());
}

StringSplitter::const_iterator StringSplitter::end() const {
    logMessage("CV End called!\n");
    return const_iterator(pattern, string_to_split.end(), string_to_split.end());
}

StringSplitter::const_iterator StringSplitter::cend() {
    logMessage("Const End called!\n");
    return const_iterator(pattern, string_to_split.end(), string_to_split.end());
}
\$\endgroup\$
  • \$\begingroup\$ A lot of excellent points. My use of this comes from experience with Java. I didn't make the constructor for the StringSplitter class because I felt it was functioning more as a utility class than a concrete class. If this was python the class would have been filled with @classmethod methods. I will start refactoring once I get the chance. Thanks for the input. Hopefully more people give suggestions \$\endgroup\$ – smac89 Apr 28 '16 at 20:23

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.