7
\$\begingroup\$

(See the next iteration.)

I have this small template function for convenient dumping a sequence into a string such that there is a delimeter between two consecutive elements, and no such after the last element:

#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;

template<typename T>
std::string join(const T begin,
                 const T end,
                 std::string separator,
                 std::string concluder)
{
    const auto length = std::distance(begin, end);
    std::stringstream ss;
    size_t count = 0;

    for (T iter = begin; iter != end; ++iter, ++count)
    {
        ss << *iter;

        if (count < length - 1)
        {
            ss << separator;
        }
    }

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

template<typename T>
std::string join(T begin, T end, std::string separator)
{
    return join(begin, end, separator, "");
}

template<typename T>
std::string join(T begin, T end)
{
    return join(begin, end, ", ");
}

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;
             });
}

Any idea how to improve this?

\$\endgroup\$
  • \$\begingroup\$ Yes, don't write something like that. Join the lines of a matrix into a single range. I think Boost has facilities for this, or you can use Eric Niebler's ranges library. \$\endgroup\$ – einpoklum Sep 30 '16 at 19:27
7
\$\begingroup\$

Your code is fine, except for three small quirks

  1. You don't need overloads if you provide default arguments
  2. You don't need constant Ts
  3. You don't need to check the length of your collection (except for begin == end).
template<typename InputIt>
std::string join(InputIt begin,
                 InputIt end,
                 const std::string & separator =", ",  // see 1.
                 const std::string & concluder ="")    // see 1.
{
    std::ostringstream ss;

    if(begin != end)
    {
        ss << *begin++; // see 3.
    }    

    while(begin != end) // see 3.
    {
        ss << separator;
        ss << *begin++;
    }

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

You only need to check whether you have an empty range. If you don't, you start with the first one, and then stream the separator before the next element:

(0 elements case) <concluder>
(1 element  case) <element> <concluder>
(2 elements case) <element> <separator> <element> <concluder>
(3 elements case) <element> <separator> <element> <separator> <element> <concluder>

As you can see, except for the first element, you will always get <separator> <element> for any other element. This adds a little bit more logic to the empty case, but you don't need to traverse your range twice, and this method makes your algorithm eligible for input iterators.

Other than that, the only other thing I would change is to use an ostringstream instead of an stringstream. This prevents you from accidentally using >>. And global using std::*** isn't my cup of tee either. You can, however, move that into your main.

But here's a little exercise: you can just use an <iterator>:

template<typename InputIt>
std::string join(InputIt begin,
                 InputIt end,
                 const std::string & separator =", ",  // see 1.
                 const std::string & concluder ="")    // see 1.
{
    std::ostringstream ss;

    using value_type = typename std::iterator_traits<InputIt>::value_type;

    std::copy(begin, end, std::ostream_iterator<value_type>(ss, separator.c_str()));

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

However, as you've noted, it does not yield the same result as your variant. Here's the exercise, though: write an iterator that provides your behavior.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer! However, note that the ostream_iterator version will append the separator to the end of the sequence, which is against my requirements. \$\endgroup\$ – coderodde Sep 30 '16 at 16:59
  • 3
    \$\begingroup\$ @coderodde Point. Forgot about that, but it's possible to get around that with your own iterator, which is a fun exercise. Will edit my answer as soon as I'm not mobile anymore \$\endgroup\$ – Zeta Sep 30 '16 at 17:14
  • 1
    \$\begingroup\$ C++17 introduces std::ostream_joiner which exhibits the correct behavior. @JerryCoffin had a nice implementation of it called infix_iterator that was reviewed sometime ago as well. \$\endgroup\$ – Snowhawk Sep 30 '16 at 22:08
  • \$\begingroup\$ @Snowhawk04, didn't he submit it into Boost? I believe he did, and it is already available there. \$\endgroup\$ – Incomputable Sep 30 '16 at 22:34
  • \$\begingroup\$ @Olzhas not sure, but I haven't seen it in any of the typical places ostream_iterator is referenced to in boost. \$\endgroup\$ – Snowhawk Sep 30 '16 at 23:03
5
\$\begingroup\$

I assume that you probably want to users to be able to pass ForwardIterator InputIterator. I will explain why I made the assumption.

According to assumption, you can rename the T into InputIt. Iterators denoting range are usually called first and last, even though last is not actually last.

template<typename InputIt>
std::string join(InputIt first,
                 InputIt last,
                 std::string separator,
                 std::string concluder)

I've never seen iterators being passed as const. Probably you wanted iterators referring to const objects? It is common to say const iterator, but it doesn't mean that iterators are const.

count and length variables are somewhat odd. The pair of iterators should denote the range needed, and it is work of the operator!=() to check if the end is hit.

for (T iter = begin; iter != end; ++iter, ++count)

Never seen traversing the range like that. People usually use this:

while (first != last)
{
    ss << *iter++;
    ss << separator;
}

It will need to be tweaked a bit to not output the separator at the end. I'll use what @Zeta suggested (awesome idea):

We'll check if the range is empty, then print the first element:

if (first == last)
{
    return concluder;
}

std::stringstream ss;
ss << *first;
++first;

And then we swap sequence of output in the loop, so we output separator first, then the element. This way, there won't be separator at the end:

while (first != last)
{
    ss << separator;
    ss << *first;
    ++first;
}

Even though preincrement is slightly faster than postincrement, my benchmarking program didn't show significant difference (it is less than 1%). Nevertheless, I've used preincrement to not make life harder for custom iterators.

Then we simply output concluder:

ss << concluder;

Rather than creating multiple overloads use default arguments:

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

And const correctness, of course.

Full code:

#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;

template<typename InputIt>
std::string join(InputIt first,
    InputIt last,
    const std::string& separator = ", ",
    const std::string& concluder = "")
{
    if (first == last)
    {
        return concluder;
    }

    std::stringstream ss;
    ss << *first;
    ++first;

    while (first != last)
    {
        ss << separator;
        ss << *first;
        ++first;
    }

    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;
    });
}

I don't really like using, but I don't think it is of any importance here.

Using input itertors will yield basic exception safety, since the range is invalid after it being passed once. For other supported iterator types the function has strong exception safety guarantee.

\$\endgroup\$
  • \$\begingroup\$ Instead of using preend, you can simply print the *first++ and then swap both ss << lines in your loop. And if you replace std::distance(...) == 0 with first != last, you can use InputIt again. Also, prev needs BidirectionalIt. \$\endgroup\$ – Zeta Sep 30 '16 at 15:46
  • \$\begingroup\$ @Zeta, thank you! I edited it into the post. \$\endgroup\$ – Incomputable Sep 30 '16 at 15:55
  • \$\begingroup\$ Nix the inline post-increments. In addition to being an obtuse syntax, post-increment on iterators often involves runtime overhead compared to pre-increment (at least in debug builds). If you don't want to add two extra lines, while (++first != last) would also do the trick, while avoiding post-increment. \$\endgroup\$ – bcrist Oct 1 '16 at 0:33
  • \$\begingroup\$ @bcrist, I've made a benchmark. The numbers I got on the last run were 356.15 (current) and 356.05("fast") milliseconds. The benchmark consisted from generating 1M random integers in range of 0-100 and throw them into the function, get average of 20 runs. I've benchmarked an std::list too, got 364.35 (current) and 362.9 ("fast"). Difference is less than 1%. On top of that, no sane person will rely on performance of Debug build. Compiler flags: /02, prefer fast code. \$\endgroup\$ – Incomputable Oct 1 '16 at 1:10
  • \$\begingroup\$ So the question is: would one use the function with that much data? I don't think so. Lets leave performance issues unless they are important. Current code has good performance. \$\endgroup\$ – Incomputable Oct 1 '16 at 1:14

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.