2
\$\begingroup\$

The feature

Here is a Grid class representing a 2d grid.
The class will get templated once it reach a satisfactory state.
At this time the cells are int values.

This Grid class allow for iteration by rows using simple range-for syntax.

    Grid grid(8, 8);

    for (auto&& row_it : grid.Rows())
    {
        for (auto&& cell : row_it)
            std::cout << std::setw(2) << std::setfill('0') << cell << ',';
        std::cout << '\n';
    }

The retrospective

It works well, but it is my first time creating custom iterators and I do not think that its is nicely done.

More in particular:

  • Are the classes GridRows, RowsIterator and RowIterator really needed?
  • The range-for will automatically call operator* on the type returned from begin(), which seems to force the existence of RowIterator instead of returning directly an int* from RowsIterator::begin
  • How best to minimize and simplify?

Standard library compatibility

According to this article, custom iterators should include the following properties: https://www.internalpointers.com/post/writing-custom-iterators-modern-cpp

  • iterator_category
  • difference_type
  • value_type
  • pointer
  • reference

The complete code

#include <iostream>
#include <iomanip>

struct RowIterator
{
    int* _it, * _end;

    RowIterator(int* begin, int* end) :
        _it{ begin },
        _end{ end }
    {}

    int& operator*()
    {
        return *_it;
    }

    int* operator->()
    {
        return _it;
    }

    RowIterator& operator++()
    {
        ++_it;
        return *this;
    }

    RowIterator operator++(int)
    {
        auto self = *this;
        ++* this;
        return *this;
    }

    friend bool operator==(RowIterator lhs, RowIterator rhs)
    {
        return lhs._it == rhs._it;
    }

    friend bool operator!=(RowIterator lhs, RowIterator rhs)
    {
        return lhs._it != rhs._it;
    }

    int* begin()
    {
        return _it;
    }

    int* end()
    {
        return _end;
    }
};

struct RowsIterator
{
    int* _it, * _end;
    int _row_length;
    RowIterator _row_begin, _row_end;

    RowsIterator(int* begin, int row_length) :
        _it{ begin },
        _end{ begin + row_length },
        _row_length{ row_length },
        _row_begin{ _it, _end },
        _row_end{ _end, _end }
    {}

    RowIterator& operator*()
    {
        return _row_begin;
    }

    RowIterator* operator->()
    {
        return &_row_begin;
    }

    RowsIterator& operator++()
    {
        _it += _row_length;
        _end += _row_length;
        _row_begin = { _it, _end };
        _row_end = { _end, _end };
        return *this;
    }

    RowsIterator operator++(int)
    {
        auto self = *this;
        ++* this;
        return *this;
    }

    friend bool operator==(RowsIterator lhs, RowsIterator rhs)
    {
        return lhs._it == rhs._it;
    }

    friend bool operator!=(RowsIterator lhs, RowsIterator rhs)
    {
        return lhs._it != rhs._it;
    }

    RowIterator begin() const
    {
        return _row_begin;
    }

    RowIterator end() const
    {
        return _row_end;
    }
};

struct GridRows
{
    RowsIterator _begin, _end;

    GridRows(int* begin, int* end, int row_length) :
        _begin{ begin, row_length },
        _end{ end, row_length }
    {}

    RowsIterator begin()
    {
        return _begin;
    }

    RowsIterator end()
    {
        return _end;
    }
};

struct Grid
{
    Grid(int width, int height) : _width{ width }, _height{ height }
    {
        size_t size = width * height;
        _begin = new int[size];
        _end = _begin + size;

        InitializeValues();
    }

    ~Grid()
    {
        if (_begin)
        {
            delete[] _begin;
            _begin = nullptr;
            _end = nullptr;
        }
    }

    void InitializeValues()
    {
        for (int i = 0; auto && it : *this)
            it = ++i;
    }

    int* begin() const { return _begin; }
    int* end() const { return _end; }

    GridRows Rows() const
    {
        return GridRows{ _begin, _end, _width };
    }

    int* _begin, * _end;
    int _width, _height;
};

void IterateWithFor(const Grid& grid)
{
    std::cout << "Iterate though all rows cells\n";
    auto&& rows = grid.Rows();
    for (auto&& row_it = rows.begin(); row_it != rows.end(); ++row_it)
    {
        for (auto&& cell = row_it.begin(); cell != row_it.end(); ++cell) {
            std::cout << std::setw(2) << std::setfill('0') << *cell << ',';
        }
        std::cout << '\n';
    }
    std::cout << '\n';
}

void IterateWithRangeFor(const Grid& grid)
{
    std::cout << "Iterate though all rows cells\n";
    for (auto&& row_it : grid.Rows())
    {
        for (auto&& cell : row_it)
            std::cout << std::setw(2) << std::setfill('0') << cell << ',';
        std::cout << '\n';
    }
    std::cout << '\n';
}

int main()
{
    Grid grid(8, 8);
    IterateWithFor(grid);
    IterateWithRangeFor(grid);
    return 0;
}

Thank you

Thank you very much for your time and valuable feedback.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
RowIterator operator++(int)
{
    auto self = *this;
    ++* this;
    return *this;
}

We should be returning the previous value (self), rather than *this.


Grid owns storage using a raw pointer, so needs copy/move constructor and assignment operations. Or better, use a std::vector instead of int* to take care of that automatically.


Use std::size_t for width and height, rather than int. That way, we don't need to check that they are not negative.


The iterators are missing the necessary type names to be used with standard algorithms. You even mention that in the question, so fix that immediately.


Iterator's operator== can be defined = default, and that will also default !=.


RowIterator begin() const
{
    return _row_begin;
}

That's dangerous - we should be returning a const-iterator when the object is const. Also consider implementing the other (const/reverse) begin/end functions.


~Grid()
{
    if (_begin) …
}

I don't see any way that _begin could be false here.


int* begin() const { return _begin; }
int* end() const { return _end; }

Again, we need to sort out the const-correctness here.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you, I am still working on simplifying it and its taking some time. I will post a new version when its ready. Your comments have been integrated. \$\endgroup\$ Nov 25, 2021 at 3:56
  • \$\begingroup\$ The new version is ready but not yet satisfactory. It is posted here: codereview.stackexchange.com/questions/270693/… \$\endgroup\$ Dec 5, 2021 at 4:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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