4
\$\begingroup\$

This structure is supposed to iterate over the lines of an input stream, using std::getline. Any remark is welcome!

#include <iostream> //istream
#include <cassert>

struct line_iterator
{
    explicit
    line_iterator( std::istream & is )
        : m_stream( &is )
        , m_line( extract_line() )
    {
    }

    line_iterator()
        : m_stream( nullptr )
        , m_line()
    {
    }

    line_iterator( line_iterator & ) = default;

    std::string operator*()
    {
        assert( m_stream );
        return m_line;
    }

    line_iterator operator++()
    {
        assert( m_stream );
        update_stream();
        return *this;
    }

    line_iterator operator++(int)
    {
        line_iterator copy( *this );
        ++(*this);
        return copy; 
    }
private:
    void update_stream()
    {
        if ( m_stream->good() )
            m_line = extract_line();

        else
        {
            m_line.clear();
            m_stream = nullptr;
        }
    }

    std::string extract_line()
    {
        std::string line;
        std::getline( *m_stream, line );
        return line;
    }

private:
    std::istream *  m_stream;
    std::string     m_line;

    friend bool operator!=( const line_iterator&, const line_iterator& );
    friend bool operator==( const line_iterator&, const line_iterator& );
};

inline
bool operator!=( const line_iterator & a, const line_iterator & b )
{
    return !(a.m_stream == b.m_stream);
}

inline
bool operator==( const line_iterator & a, const line_iterator & b )
{
    return a.m_stream == b.m_stream;
}
\$\endgroup\$
  • \$\begingroup\$ The standard already has a built-in technique for iterating over stream. std::istream_iterator<Type>. So you can iterate through a stream by object of Type. If you need to iterate by line then just define what that means in a type called Line. \$\endgroup\$ – Martin York Mar 27 '15 at 17:50
2
\$\begingroup\$
  1. You might consider providing typedefs for the types expected by std::iterator_traits.

  2. Your type should meet the requirements of InputIterator. This means that you should also provide an overload for operator ->.

  3. The semantics of operator != are not really useful. For example, this won't allow you to use your iterator in the idiomatic

    line_iterator begin = …;
    line_iterator end = …;
    while (begin != end)
      {
        std::cout << *begin << std::endl;
        ++begin;
      }
    

    because there is no value you can set end to such that it compares not non-equal to begin only after begin was advanced a certain number of times. I suggest you implement it somehow like this:

    friend bool
    operator!=(const line_iterator& lhs, const line_iterator& rhs) noexcept
    {
      if (lhs.m_stream == nullptr && rhs.m_stream == nullptr)
        return false;
      else if (lhs.m_stream == nullptr)
        return rhs.m_stream->good();
      else if (rhs.m_stream == nullptr)
        return lhs.m_stream->good();
      else
        return lhs.m_stream != rhs.m_stream;
    }
    

    This will allow you to use a default-constructed line_iterator as end and have it compare not non-equal to begin only after begin's stream is exhausted.

    You should then implement lhs == rhs as !(lhs != rhs).

  4. You are making many copies of the string which is not as efficient as it could be. I also feel that it makes your code more complicated than necessary. You should read directly into m_string by implementing the increment operator as

    line_iterator&
    operator++()
    {
      assert(m_stream && m_stream->good());
      std::getline(*m_stream, m_string);
      return *this;
    }
    

    and the dereferencing operator as

    const std::string&
    operator*() const
    {
      return m_string;
    }
    

    In your constructor, default-construct m_string and (if the pre-conditions are met) advance once.

  5. Personally, I prefer to only mention those members in the constructor's initializer list that depend on the constructor arguments. Those that get default values I prefer to initialize in the class definition.

    class line_iterator
    {
    
    public:
    
      line_iterator()
      {
        // nothing to do
      }
    
      line_iterator(std::istream& istr) : m_stream {&istr}
      {
        if (m_istr->good())
          std::getline(*m_stream, m_string);
      }
    
      …
    
    private:
    
      std::string m_string {};     // defaults to empty string
      std::istream * m_stream {};  // defaults to 'nullptr'
    };
    
  6. If you're going to make the friends inline anyway, I'd prefer to define them right in the class body.

\$\endgroup\$
2
\$\begingroup\$

There is a much easier way of doing this:

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

struct Line
{
     std::string   lineData;
     // Optional Way to convert a Line into std::string
     // operator std::string&() {return lineData;}
     friend std::istream& operator>>(std::istream& stream, Line& data)
     {
         return std::getline(stream, data.lineData);
     }
     friend std::ostream& operator<<(std::ostream& stream, Line const& data)
     {
         return stream << data.lineData << "\n";
     }
};

int main()
{
    std::copy(std::istream_iterator<Line>(std::cin),
              std::istream_iterator<Line>(),
              std::ostream_iterator<Line>(std::cout, "Line: -> ")
             );
}
\$\endgroup\$

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.