1
\$\begingroup\$

I'm afraid just about everybody who read this question has misunderstood it. I'm not trying to find the best way of printing a vector. I'm trying to find the best way of excluding part of an operation from the last iteration in a loop. I'm seeking advice about a pattern that is repeated is many different contexts. For this reason, I cannot provide a concrete example.

This question is off-topic


A well-formed comma-separated list has commas after all but the last element. This means that when dealing with these lists, the comma has to be dealt with on all but the last iteration. This is how I do things currently:

for (auto e = vec.cbegin(); e != vec.cend(); ++e) {
  // code that has to be executed for every element
  std::cout << *e;
  if (e != vec.cend() - 1) {
    // code that must not be executed for the last element
    std::cout << ", ";
  }
}

There are five instances of the above pattern in the project that I'm working on. I feel like there might be a more elegant way of doing this. An alternative that I've come up with is this:

// infinite loop if vec is empty
if (!vec.empty()) {
  for (auto e = vec.cbegin(); e != vec.cend() - 1; ++e) {
    // code that has to be executed for every element
    std::cout << *e;
    // code that must not be executed for the last element
    std::cout << ", ";
  }
  // code that has to be executed for every element
  std::cout << vec.back();
}

This alternative is slightly faster because there isn't an if in the body but that's not a big deal. The if is true for all but the last iteration so the branch predictor will probably only miss on the last iteration. I also have to repeat code that has to be executed for every element which means that I need to hoist that into a function.

The first snippet is the best that I can come up with but I feel like this could be done better. Note that writing to std::cout is just an example.

\$\endgroup\$

closed as off-topic by Kerndog73, 200_success, Simon Forsberg Nov 6 '18 at 23:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Kerndog73, 200_success, Simon Forsberg
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ You're implementing an ostream_joiner - is this reinventing-the-wheel, or do you need something for straight C++17 without std::experimental? \$\endgroup\$ – Toby Speight Nov 6 '18 at 11:27
  • 1
    \$\begingroup\$ So this is not the actual code you have written or are maintaining? Clearly off-topic, I'm afraid. \$\endgroup\$ – Toby Speight Nov 6 '18 at 11:35
  • 1
    \$\begingroup\$ Damn, this place is hostile \$\endgroup\$ – Kerndog73 Nov 6 '18 at 11:42
  • 3
    \$\begingroup\$ @Kerndog73, if you're talking about me, I'm not intending to be hostile, and apologies if you perceive things that way. I think you may well be able to salvage an on-topic question from this, and there's some very worthwhile observations I'd be able to make if I could see an actual context. It really helps to have compilable code - it's worth writing a small main() to exercise a function you've made for your project, for example, and you should definitely include the headers and/or definitions you use. I want this to be a good, on-topic question, and it's frustrating that it's not (yet). \$\endgroup\$ – Toby Speight Nov 6 '18 at 12:32
  • 3
    \$\begingroup\$ @Kerndog73 I’m honestly confused by the rules as well. This question might be better suited for softwareengineering.stackexchange.com (what a mouthful, that URL). But I’m not absolutely sure it is, and I believe there should be a room for such a question somewhere. \$\endgroup\$ – Konrad Rudolph Nov 6 '18 at 13:51
13
\$\begingroup\$

If you are just doing a single-shot printing of a container, just print the first element and then print delim-value pairs. This reduces the iterator requirements to being equality comparable, dereferenceable, and incrementable.

if (!vec.empty()) {  // invert to return immediately if this is a function
    auto first = vec.cbegin();
    std::cout << *first;

    while (vec.cend() != ++first) {
        std::cout << ", " << *first;
    }
}

There is also a stateful approach that reduces the operation strength from branching on whether to print the delimiter to a pointer assignment.

auto actual_delim = ", ";
auto delim = "";

for (const auto& elem : vec) {
    std::cout << delim << elem;
    delim = actual_delim;
}

See infix_iterator where this stateful approach is used.

\$\endgroup\$
  • \$\begingroup\$ As mentioned in my answer, this approach has the distinct advantage of duplicating the logic of the loop body. \$\endgroup\$ – Konrad Rudolph Nov 6 '18 at 13:53
  • \$\begingroup\$ Second version is much more appealing, as we don't lose range-based for (and you can set a function pointer or plain bool variable instead of a string-pointer if necessary). In a worst case, we could test &elem == &vec.back() if we can't make the first element have the variant behaviour (e.g. joining a list with " and " as the final separator). \$\endgroup\$ – Toby Speight Nov 6 '18 at 17:39
5
\$\begingroup\$

I'm not sure that elegance should be your first concern here, because the algorithm should be very simple and the code short however you write them.

I/O operations will have a much greater impact on performance than the lay-out of your loop, so you shouldn't worry too much about speed either.

Generality, I believe, is paramount in this case. That's why I see vec.cend() - 1 as the main short-coming of your proposal, because it restricts your algorithm to at least bidirectional iterators, when your goal can be easily achieved for any kind of iterator.

Here's what I would consider better:

#include <iostream>
#include <vector>

template <typename Container, typename Separator> // any container is acceptable...
void pretty_print(const Container& c, const Separator& s) {
    std::cout << '{';
    auto it = std::begin(c); // ... even arrays
    if (it != std::end(c)) do {
        std::cout << *it;
        if (++it == std::end(c)) break; // that way you don't need to decrement the iterator
        std::cout << s;
    } while (true);
    std::cout << '}';
}
\$\endgroup\$
  • \$\begingroup\$ I did mention that writing to std::cout was just an example but I do like that this works with forward iterators. \$\endgroup\$ – Kerndog73 Nov 6 '18 at 8:57
  • \$\begingroup\$ I think I have to agree with Calak (answer below): this variant is decidedly less readable than others. But it would help to make it equivalent to what OP is doing to have a direct comparison (i.e. remove the printing of the braces). \$\endgroup\$ – Konrad Rudolph Nov 6 '18 at 10:08
3
\$\begingroup\$

I always just use an additional variable:

auto first= true;

for (auto const& x : list) {
    if (first) first = false; else separator(x);
    action(x);
}

In your case, separator(x) would be std::cout << ", " (x isn’t actually used), and action(x) would be std::cout << x.

I normally abhor mutating variables inside a loop and try to use standard library algorithms on lambdas without side-effects instead. But in this case I believe having an additional boolean flag is simply the most economical, most readable solution.

This is also one of the very few cases (the only?) where I’m using a single-line ifelse.

The advantage of this method is that you don’t have to duplicate action(x). Even if it’s just a single, simple expression, having such a duplication makes the logic disjointed and harder to follow.

The disadvantage is that the condition first has to be checked on every loop iteration. My suspicion is that the CPU’s branch predictor will handle this extremely well so it shouldn’t impact performance. Still, it’s conceptually irksome.

\$\endgroup\$
  • \$\begingroup\$ As always I’d love an explanation for downvotes. \$\endgroup\$ – Konrad Rudolph Nov 6 '18 at 13:50
1
\$\begingroup\$

We are getting closer to the off-topic "opinion-based" border

What's readable is a matter of taste. Honestly, i find the @papafaga version less fluent. You also have the possibility to do the opposite of your second version, to put the particular case before the loop, I do it sometimes when the case is trivial.

template <typename Container, typename Separator>
void pretty_print4(const Container& c, const Separator& s) {
    if (c.empty()) return;

    auto it = std::begin(c); 
    std::cout << *it;
    std::advance(it,1);
    while (it != std::end(c)) {
        std::cout << s;
        std::cout << *it;
        std::advance(it,1);
    } 
}

If you talk about performance, I think all versions are worth. Without optimizations, some versions generate more instructions than others, but once in -O3, except one or two instructions, it's almost the same. After, if it really matters, you have to look at the machine code and benchmark.

\$\endgroup\$
  • \$\begingroup\$ Why use std::advance to simply increment by 1? Any iterator has opertor++. \$\endgroup\$ – Ruslan Nov 6 '18 at 21:04
  • \$\begingroup\$ Two reason: It's more explicit and this \$\endgroup\$ – Calak Nov 6 '18 at 21:25
  • 3
    \$\begingroup\$ @Calak The reasoning in your link only makes sense for increment values ≠ 1. And std::advance is not more explicit than using ++. The latter is precisely defined and forms part of the iterator contract. It couldn’t be more explicit. \$\endgroup\$ – Konrad Rudolph Nov 6 '18 at 23:02
1
\$\begingroup\$

I would use an infix iterator:

Example here: https://stackoverflow.com/q/3496982/14065

std::copy(vec.begin(), vec.end(), infix_iterator(std::cout, ", "));

Or if you ahve the latest version of the compiler:

https://en.cppreference.com/w/cpp/experimental/ostream_joiner

 std::copy(vec.begin(), vec.end(), std::ostream_joiner(std::cout, ", "));
\$\endgroup\$
  • \$\begingroup\$ Please read the first paragraph of the question \$\endgroup\$ – Kerndog73 Nov 6 '18 at 21:50
  • \$\begingroup\$ @Kerndog73: I did. Does not change my answer. Still the best way to do it. Of course you need to customize the iterator for your situation but the same principles still applies. You are applying a standard action to each element and an extra action to all but the last element. \$\endgroup\$ – Martin York Nov 6 '18 at 21:55
  • \$\begingroup\$ So you're suggesting I make a special iterator that accepts a couple of lambdas and iterates a container? \$\endgroup\$ – Kerndog73 Nov 6 '18 at 21:57
  • \$\begingroup\$ @Kerndog73: You are applying an action to all elements. Then you are applying another action to all but the last element. Sounds like an algorithmic processes that is covered by the above. \$\endgroup\$ – Martin York Nov 6 '18 at 22:00
  • \$\begingroup\$ After all that has been said by all the answers. I think I'm just going to keep doing things the way that I'm doing them. \$\endgroup\$ – Kerndog73 Nov 6 '18 at 22:03
0
\$\begingroup\$

I would treat the work to do as [head] followed by [tail]. The head item is not preceded by a separator while all tail items are preceded by a separator. The code can use this difference. The pseudocode then looks something like:

// Process head.
processHeadItem()

// Process tail
repeat
  processSeparator()
  processNextTailItem()
until (tail all processed)

In real code you could start the tail processing loop with for (int i = 1, ... since i = 0 is the head item.

\$\endgroup\$
  • \$\begingroup\$ This was already suggested by Calak and Snowhawk \$\endgroup\$ – Kerndog73 Nov 6 '18 at 22:09
  • \$\begingroup\$ Yes, but I thought my presentation was clearer, more general and gave some of the theory behind the solution: [Head], [Tail]. \$\endgroup\$ – rossum Nov 6 '18 at 22:43

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