Simple string joiner in modern C++ - follow-up

Question

(See the previous iteration.)

Now, I have defined an iterator for dumping contents of a sequence such that the last element is not followed by the separator string.

main.cpp:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <sstream>
#include <string>
#include <vector>

using std::cout;
using std::endl;
using std::for_each;
using std::vector;

using namespace std;

template <typename T, typename charT=char, typename traits=char_traits<charT>>
class my_ostream_iterator :
public iterator<output_iterator_tag, void, void, void, void>
{
    std::ostringstream* oss;
    std::string* separator;
    bool first_value_appended;

public:

    my_ostream_iterator(std::ostringstream* oss_, std::string* separator_)
    :
    oss{oss_},
    separator{separator_},
    first_value_appended{false}
    {}

    my_ostream_iterator<T,charT,traits>& operator= (const T& value) {
        if (!first_value_appended)
        {
            *oss << value;
            first_value_appended = true;
        }
        else
        {
            *oss << *separator << value;
        }

        return *this;
    }

    my_ostream_iterator<T,charT,traits>& operator*()     { return *this; }
    my_ostream_iterator<T,charT,traits>& operator++()    { return *this; }
    my_ostream_iterator<T,charT,traits>& operator++(int) { return *this; }
};

template<typename InputIt>
std::string join(InputIt begin,
                 InputIt end,
                 const std::string& separator =", ",
                 const std::string& concluder ="")
{
    std::ostringstream ss;
    std::string sep = separator;
    using value_type = typename std::iterator_traits<InputIt>::value_type;

    std::copy(begin,
              end,
              my_ostream_iterator<value_type>(&ss, &sep));

    ss << concluder;
    return ss.str();
}

int main() {
    vector<vector<int>> matrix = {
        { 1, 2, 3 },
        { 4, 5 },
        { },
        { 10, 26, 29 }
    };

    for_each(matrix.cbegin(),
             matrix.cend(),
             [](std::vector<int> a) {
                 cout << join(a.cbegin(), a.cend()) << endl;
             });
}

As always, any critique is much appreciated.

Answer 1

using declarations

Don't do this at top level:

using namespace std;

and certainly not in a header file, where it can can bring unexpected names into user code.

using std::cout;
using std::endl;
using std::for_each;
using std::vector;

These are less dangerous, but should still be avoided in headers. Reduce their scope and visibility as much as you can, or use fully-qualified names.

Pointers and ownership

std::ostringstream* oss;
std::string* separator;

Who owns these objects? Can they be null? oss is used without checking for null, and it does not appear to be owned by this, so it should probably be a reference. separator should probably be an owned value, so you can initialize from an rvalue.

Type of separator

Why is separator a std::string? If the output stream can handle wide strings, we should be able to separate our values with wide strings.

Usual operator types

g++ -Weffc++ recommends that you return the result of postincrement by value, rather then by reference. That's arguably moot in this case, as incrementing is a no-op, but I like to avoid warnings!

Code duplication

It's a small duplication, but I think it's clearer to separate out the emitting of separator and value:

my_ostream_iterator<T,charT,traits>& operator= (const T& value) {
    if (first_value_appended)
        oss << separator;
    else
        first_value_appended = true;

    oss << value;
    return *this;
}

Generality

Your iterator is templated on the character type, but you don't seem to make use of that type (see "type of separator" above). I prefer to make the output stream a std::basic_ostringstream<CharT, Traits> so that your code will produce the correct kind of string as output.

Efficiency

In your std::for_each in main(), you pass a vector by value. It should probably be a reference to const vector.

---

Modified code

#include <iterator>
#include <sstream>
#include <string>

template<typename T, typename CharT=char, typename Traits=std::char_traits<CharT>>
class my_ostream_iterator
    : public std::iterator<std::output_iterator_tag, void, void, void, void>
{
    using string_t = std::basic_string<CharT, Traits>;
    using stream_t = std::basic_ostringstream<CharT, Traits>;

    stream_t& oss;
    const string_t separator;
    bool first_value_appended;

public:

    my_ostream_iterator(stream_t& oss, const string_t& separator)
        : oss{oss},
          separator{separator},
          first_value_appended{false}
    {
    }

    my_ostream_iterator<T,CharT,Traits>& operator= (const T& value) {
        if (first_value_appended)
            oss << separator;
        else
            first_value_appended = true;

        oss << value;
        return *this;
    }

    my_ostream_iterator<T,CharT,Traits>& operator*()     { return *this; }
    my_ostream_iterator<T,CharT,Traits>& operator++()    { return *this; }
    my_ostream_iterator<T,CharT,Traits> operator++(int)  { return *this; }
};

template<typename InputIt, typename StringT = std::string>
StringT join(InputIt begin,
             InputIt end,
             const StringT& separator = ", ",
             const StringT& concluder = {})
{
    using value_type = typename std::iterator_traits<InputIt>::value_type;
    using char_type = typename StringT::value_type;

    std::basic_ostringstream<char_type> ss;
    std::copy(begin, end,
              my_ostream_iterator<value_type, char_type>(ss, separator));

    ss << concluder;
    return ss.str();
}


// test program
#include <algorithm>
#include <iostream>
#include <vector>

int main() {
    // Bring in names needed for argument-dependent lookup
    using std::begin;
    using std::end;

    const std::vector<std::vector<int>> matrix = {
        { 1, 2, 3 },
        { 4, 5 },
        { },
        { 10, 26, 29 }
    };

    std::for_each(begin(matrix), end(matrix),
                  [](const auto& a) {
                      std::cout << join(begin(a), end(a)) << std::endl;
                  });
    std::cout << " ---" << std::endl;

    std::for_each(begin(matrix), end(matrix),
                  [](const auto& a) {
                      std::wcout << join(begin(a), end(a), std::wstring(L", ")) << std::endl;
                  });
    std::wcout << " ---" << std::endl;
}

Answer 2

using std::cout;
using std::endl;
using std::for_each;
using std::vector;

using namespace std;

Be aware of the pitfalls of using declarations and directives. Whether at global scope or function level scope, importing every symbol available in a namespace may cause collisions and ambiguity. The general advice is do not use using directives (using namespace X). using declarations should be kept local to functions and namespaces to minimize the amount of coverage in which a collision might occur.

---

class my_ostream_iterator 

Naming is important. Reading my_ostream_iterator, I would expect to be able to use it with any std::ostream derived object. Instead, it only works with objects derived from std::ostringstream.

Your class is also named similarly to the std::ostream_iterator that already exists in the standard library. As someone aware of its existence, I might assume that the behavior would be similar. You can pick a better name here.

class my_osstream_joiner

---

: public iterator<output_iterator_tag, void, void, void, void>

std::iterator is being deprecated in C++17 as it does not provide code clarity and does not operate with internal name lookup. You should just provide the 5 typedefs of std::iterator yourself.

---

std::ostringstream* oss;
std::string* separator;

Can they point to nullptr? What happens if they do? Your options are either to check for null on oss assignment/use, contractually mandate oss not be null (gsl::not_null<T>, std::reference_wrapper<T>), or use a plain reference.

---

my_ostream_iterator(std::ostringstream* oss_, std::string* separator_)
: oss{oss_}
, separator{separator_}
, first_value_appended{false}
{}

When assigning data members with constant values, prefer default member initialization.

class my_ostream_iterator {
    std::ostringstream* oss{nullptr};
    std::string* separator{nullptr};
    bool first_value_appended{false};
public:
    my_ostream_iterator(...) 
    : oss{oss_}
    , separator{separator_} 
    {}

---

my_ostream_iterator<T,charT,traits>& operator= (const T& value) {
    if (!first_value_appended) {
        *oss << value;
        first_value_appended = true;
    } else {
        *oss << *separator << value;
    }
    return *this;
}

There is an opportunity for strength reduction by weakening your comparisons into pointer assignments.

Does your iterator need to be templated? You might want to mimic the existing iterator adaptors that require all values assigned to be the same as or convertible to T. That is fine. If you want to loosen that restriction and provide more flexibility, you can template the assignment operator to take any streamable type instead.

class my_ostream_iterator {
    std::ostringstream* oss{nullptr};
    std::string* printed_separator{nullptr}; // not assigned to in ctor
    std::string* actual_separator{nullptr};
public:
    template <typename OStreamable>
    my_ostream_iterator& operator=(const OStreamable& value) {
        *oss << printed_separator << value;
        printed_separator = actual_separator;
        return *this;
    }

---

template<typename InputIt>
std::string join(InputIt begin,
                 InputIt end,
                 const std::string& separator =", ",
                 const std::string& concluder ="")

Must separator be a std::string? Perhaps allow customization on the character type.

If you are going to provide a suffix decorator customization point, consider providing a prefix decorator as well.

---

for_each(matrix.cbegin(),
         matrix.cend(),
         [](std::vector<int> a) {
             cout << join(a.cbegin(), a.cend()) << endl;
         });

Avoid std::endl as it does more than stream the end-of-line character. If you want the std::flush behavior, explicitly stream std::flush to indicate to the reader that it was intentional.

Careful with your passing-by-value in the lambda. Prefer to use auto instead of explicitly typed variables to minimize your commitment to details (minimize rigidity).