Input iterator in C++

For educational purposes and to understand how iterators are working in C++, I've create an input iterator and I would like others to look at and provide any feedback on my code.

template
class iter_input {
 public:
  typedef T         value_type;
  typedef T&        reference_type;
  typedef const T&  const_reference_type;
  typedef T*        pointer_type;
  typedef const T*  const_pointer_type;

  iter_input(pointer_type& pointer) : pointer_(pointer) {}
  iter_input(const iter_input& other) : pointer_(other.pointer_) {}

  iter_input& operator= (const iter_input& rhs) {
    this->pointer_ = rhs.pointer_;
    return (*this);
  }

  iter_input& operator++ (void) {
    ++this->pointer_;
    return (*this);
  }

  iter_input operator++ (int) {
    iter_input temp(*this->pointer_);
    ++*this;
    return (temp);
  }

  bool operator== (const_reference_type rhs) {
    return (this->pointer_ == rhs.pointer_);
  }

  bool operator!= (const_reference_type rhs) {
    return (this->pointer_ != rhs.pointer_);
  }

 private:
  T* pointer_;
};

References:

• apache.org

• sgi.com

Correctness Issues

Needs dereferencing operator

There are two basic operations every iterator needs to support:

• Incrementing the iterator

• Dereferencing the iterator

Right now, you've covered incrementing, but not dereferencing. That means you can iterate over items, but you can't look at the items themselves, rendering the whole thing nearly useless (I would remove the "nearly", but you can compute the distance between two iterators, and once in a while that's all you really need).

reference_type operator *() { return *pointer_; }

const correctness

operator ==() and operator !=() should both be const operators.

Correct types

operator== and operator!= should take parameters of type iter_input as their parameters. When you do a comparison, you compare the iterator to another iterator, not the iterator to the type of object the iterator refers to.

These operators should look something like this:

bool operator== (iter_input rhs) const {
  return pointer_ == rhs.pointer_;
}

bool operator!= (iter_input rhs) const {
  return pointer_ != rhs.pointer_;
}

Constructor

The constructor that takes a pointer should take it by value rather than reference. This allows (for example) initializing an iterator from the name of an array:

int x[] = {1, 2, 3, 4};

iter_input begin(x);
// or: iter_input begin = std::begin(x);

You can normally take for granted that pointers are cheap to copy, so you don't normally want to pass pointers by reference for the sake of efficiency (but for things like linked lists, you do sometimes want to pass them by reference because you're going to modify the pointer that's passed).

Style Issues

this-> considered harmful

Using this-> for every reference to a member is (at least in my opinion) a poor idea. It adds visual noise, which hurts readability but provides no benefit.

return is an operator, not a function

When you return a value from a function, you don't need to enclose that expression in parentheses. Again, this simply adds visual noise and hurts readability without any benefit (and in a few cases that don't apply here can cause other problems as well).

Assignment operator

Unless you need compatibility with older (pre C++11) compilers I'd prefer to define the assignment operator as defaulted rather than including a function body for it.

iter_input& operator= (const iter_input& rhs) = default;

Use your own typedefs

Instead of T *pointer_;, I'd prefer pointer_type pointer_;.

Make it a proper header

If you want to do anything more than a minimal test of this code, you probably want it to be in a header. To facilitate that, you could add the (officially not exactly standard, but almost universally accepted) #pragma once at the beginning, or else add standard "header guards":

#ifndef ITER_INPUT_INCLUDED_
#define ITER_INPUT_INCLUDED_

// existing code here

#endif

Note that if you look at your standard headers, the name they use in place of ITER_INPUT_INCLUDED_ will typically start with an underscore. That gives a name that's reserved for the implementation, which means the people writing the compiler can (and in this case should) use that kind of name, but in normal code that's not part of the standard library, you should not use such a name.

Indentation

The 2-space indentation is too small for me to be certain what's supposed to line up with what. The access specifiers (public: and private:) being indented only one space makes this even worse. The days of 40-column displays are long gone. There's no need to skimp on indentation to this degree.

StackExchange Code Review Q#121735, answer score: 10