Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've previously posted this on Stack Overflow, and am considering submitting it to Boost for wider distribution, but thought perhaps it would be best to put it up for peer review first, and see if there are clear improvements that can be made first.

// infix_iterator.h
// 
#if !defined(INFIX_ITERATOR_H_)
#define  INFIX_ITERATOR_H_
#include <ostream>
#include <iterator>

template <class T,
          class charT=char,
          class traits=std::char_traits<charT> >

class infix_ostream_iterator :
    public std::iterator<std::output_iterator_tag,void,void,void,void>
{
    std::basic_ostream<charT,traits> *os;
    charT const* delimiter;
    bool first_elem;
public:
    typedef charT char_type;
    typedef traits traits_type;
    typedef std::basic_ostream<charT,traits> ostream_type;

    infix_ostream_iterator(ostream_type& s)
        : os(&s),delimiter(0), first_elem(true)
    {}
    infix_ostream_iterator(ostream_type& s, charT const *d)
        : os(&s),delimiter(d), first_elem(true)
    {}
    infix_ostream_iterator<T,charT,traits>& operator=(T const &item)
    {
        // Here's the only real change from ostream_iterator:
        // We don't print the delimiter the first time. After that, 
        // each invocation prints the delimiter *before* the item, not
        // after. As a result, we only get delimiters *between* items,
        // not after every one.
        if (!first_elem && delimiter != 0)
            *os << delimiter;
        *os << item;
        first_elem = false;
        return *this;
    }

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

};

#endif 

This is (at least intended to be) pretty much a drop-in replacement for std::ostream_iterator, the sole difference being that (at least in normal use) it only print out delimiters between items instead of after every item. Code using it looks something like:

#include "infix_iterator.h"

std::vector<int> numbers = {1, 2, 3, 4};

std::copy(begin(numbers), end(numbers), 
          infix_ostream_iterator<int>(std::cout, ", "));

The motivation for this is pretty simple -- with a std::ostream_iterator, your list would come out like 1, 2, 3, 4,, but with the infix_iterator, it comes out as 1, 2, 3, 4.

As a side-note, although I've used a couple of C++11 features in this demo code, I believe the iterator itself should be fine with C++03 -- though if somebody sees anything that would be a problem for a compiler without C++11 support, I'd like to hear about that too.

Edit: In case anybody cares, here's the new version incorporating input from both @Konrad and @Loki. Thanks to both of you.

// infix_iterator.h
#if !defined(INFIX_ITERATOR_H_)
#define  INFIX_ITERATOR_H_
#include <ostream>
#include <iterator>
#include <string>

template <class T, class charT=char, class traits=std::char_traits<charT> >
class infix_ostream_iterator :
    public std::iterator<std::output_iterator_tag, void, void, void, void>
{
    std::basic_ostream<charT,traits> *os;
    std::basic_string<charT> delimiter;
    std::basic_string<charT> real_delim;

public:

    typedef charT char_type;
    typedef traits traits_type;
    typedef std::basic_ostream<charT, traits> ostream_type;

    infix_ostream_iterator(ostream_type &s)
        : os(&s)
    {}

    infix_ostream_iterator(ostream_type &s, charT const *d)
        : os(&s), 
          real_delim(d)
    {}

    infix_ostream_iterator<T, charT, traits> &operator=(T const &item)
    {
        *os << delimiter << item;
        delimiter = real_delim;
        return *this;
    }

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

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

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

#endif 
share|improve this question
    
Why do you take a charT const *d instead of a const std::basic_string<charT>&? –  Dave Jul 3 '12 at 12:58
    
@Dave: To mimic the interface of std::ostream_iterator? –  dalle Jan 22 at 7:33
add comment

4 Answers

up vote 8 down vote accepted

Nothing major (just some personal opinion ones):

Consistent Spacing

    infix_ostream_iterator(ostream_type& s)
        : os(&s),delimiter(0), first_elem(true)
    {}  //    ^^^ No Space   ^^^Trailing space

Consistent type Naming

// Here we have & on the left
infix_ostream_iterator<T,charT,traits>& operator=(T const &item)

// Here we have it on the right
infix_ostream_iterator<T,charT,traits> &operator*() {

Spelling of delimter

I mistypes it a lot when writing this => delimiter

Easier to read initializer list

Just like I prefer one statement per line, I prefer one variable initialized per line in the initializer list (easier to read).

infix_ostream_iterator(ostream_type& s, charT const *d)
    : os(&s)
    , delimiter(d)
    , first_elem(true)
{}

Remove the if from the main body.

It probably makes no difference to performance but I would remove the if so the code looks like this:

*os << delimter << item;
delimter = actualDelimter;
return *this;

On construction actionDelimiter points at the string provided by the user (or empty string) and delimter is set to point at an empty string.

share|improve this answer
    
I've been thinking about this a bit. Instead of storing the user's pointer directly, and initializing delimiter to point to an empty string, do you see a problem that I don't with using std::basic_string<charT> for both delimiter and real_delim? Pros: no init to empty string needed, remain valid even if the user's pointer doesn't. Cons: Possibly increased overhead? –  Jerry Coffin Jul 1 '12 at 14:15
    
@JerryCoffin: I see no down side to that. The overhead is during construction that happens once. Even if it is copied around a lot the extra cost of the copy is not that significant. And all this is totally outweighed by the usage of operator<< –  Loki Astari Jul 1 '12 at 22:21
    
Is it really a reasonable assumption to say the copy is cheaper than the condition he previously had? Is there some way to quantify that? –  Dave Jul 3 '12 at 13:03
    
@Dave. It is not cheaper it is more expensive. But you do not expect to see much copying (as I was trying to say). So I see the extra cost as insignificant, compared to the extra safety we achieve by doing it this way. –  Loki Astari Jul 3 '12 at 15:26
    
I would leave delimiter and real_delim as charT const *, but in the constructor do as follows: infix_ostream_iterator(ostream_type &s, charT const *d = 0) : os(&s), delimiter(""), real_delimiter(d ? d : "") {}. That way there even less cost for copying. –  dalle Jan 22 at 7:30
add comment

The only thing at all that I can criticise in the code is the inconsistent placement of whitespace between infix operators and in pointer / reference declarations.

class charT=char,
public std::iterator<std::output_iterator_tag,void,void,void,void>

… etc., missing spaces.

: os(&s),delimiter(0), first_elem(true)

… etc., inconsistent use of spaces.

infix_ostream_iterator<T,charT,traits>& operator=(T const &item)
infix_ostream_iterator<T,charT,traits> &operator*() {

… etc., Inconsistent placement of &.

I’d also unify the use of blank lines between function definitions, and remove the blank line between the template <…> block and the class header, as well as at the end of the class definition.

Nitpicking, sure, but that’s literally the only thing to criticise.

share|improve this answer
add comment

I would also declare first constructor as explicit:

explicit infix_ostream_iterator(ostream_type &s)
        : os(&s)
    {}

Though I can imagine use of this kind of iterator is rather limited, this way you ensure no unwanted type conversion will take place.

share|improve this answer
add comment

Keep the const char pointers

I agree with Lokis proposed changes, except that I would leave delimiter and real_delim as charT const * and change the constructor to:

infix_ostream_iterator(ostream_type &s, charT const *d = 0)
 : os(&s)
 , delimiter("")
 , real_delim(d ? d : "")
{
}

That way we reduce the cost of copying the strings, instead we just copy a pointer.

Deduced types

Mimicking the changes made to std::less in C++14, you could make operator= a template, that way we don't need to specify this type for the class. This would though break the compatibility with the std::ostream_iterator interface.

template <class T>
infix_ostream_iterator<charT, traits> &operator=(T const &item)
{
    *os << delimiter << item;
    delimiter = real_delim;
    return *this;
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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