Sudoku Grid special purpose Iterators

Question

Please have a look at these iterators which I use for my Sudoku solver. They behave slightly different from STL iterators and don't implement all functionality that would be needed to use them in a stl context. But the basic idea behind them was to clean up the code in the Sudoku program that makes heavy use of the three access patterns (row, col, block) that I implemented.

The most "important" iterator is the BlockIterator, since without that iterating over all nine fields in a block looked quite ugly. Iterating rows and columns wasn't that bad, but since I started writing the stuff I decided to create a complete set.

Some technical details:

The grid class holds an (evil) array of pointers to Field objects, that's one dimensional (I could have used a two dimensional array as well, but I often do it this way and feel quite comfortable with modulo operations). Maybe I will replace this with a vector later.

The grid class adds a few static functions to calculate offsets in the array based on row, col or block positions.

class Grid {
public:
    Grid();
    Grid(std::string s);

    class Iterator {
    public:
        Iterator(Grid* g) : grid(g), it(0){}
        Field* operator*(){return field;}
        void operator++(){
            ++it;
            if(it < 9) field = calc_field();
            else field = NULL;
        }
    protected:
        virtual Field* calc_field() = 0;
        Field* field;
        Grid* grid;
        int it;
    };

    class RowIterator : public Iterator {
    public:
        RowIterator(Grid* g, int row) : Iterator(g){
            row_offset = row * size; //Grid::block_offset(block);
            field = calc_field();
        }
        Field* calc_field(){
            int field_index = row_offset + it;
            return grid->field[field_index];
        }
    protected:
        int row_offset;
    };

    class ColIterator : public Iterator {
    public:
        ColIterator(Grid* g, int col) : Iterator(g){
            col_offset = col;
            field = calc_field();
        }
        Field* calc_field(){
            int field_index = it * size + col_offset;
            return grid->field[field_index];
        }
    protected:
        int col_offset;
    };

    class BlockIterator : public Iterator {
    public:
        BlockIterator(Grid* g, int block) : Iterator(g){
            block_offset = Grid::block_offset(block);
            field = calc_field();
        }
        Field* calc_field(){
            int field_index = block_offset + ((it / 3) * size) + (it % 3);
            return grid->field[field_index];
        }
    protected:
        int block_offset;
    };

    RowIterator& row_iter(int row){return *(new RowIterator(this, row));}
    ColIterator& col_iter(int col){return *(new ColIterator(this, col));}
    BlockIterator& block_iter(int block){return *(new BlockIterator(this, block));}

(...)

    static int block_offset(int block){return ((block / 3) * size * 3) + ((block % 3) * 3);}

protected:
        Field* field[grid_size];

Sample usage:

This function is called, when I set a value in a field. It goes through all fields, that would possibly be influenced by this field (same row, col or block)

void Field::do_exclusions(){
    // row
    for(Grid::RowIterator it = grid->row_iter(row); *it; ++it)
        (*it)->set_excluded(value);
    // col
    for(Grid::ColIterator it = grid->col_iter(col); *it; ++it)
        (*it)->set_excluded(value);
    // block
    for(Grid::BlockIterator it = grid->block_iter(block); *it; ++it)
        (*it)->set_excluded(value);
}

So please tell me, if something like this would be "acceptable" (not to mention "best practices"), even if it somehow takes a very free view on the iterator concept.

And of course every idea how this could be improved is welcome.

PS: I tried to add a tag "iterator" but I'm not allowed for too few reputation.

Answer 1

In my first quick scan through, here are some things I want to bring up:

• If you are going to overload operators, do it the way the users of the language expect, or don't do it at all. I expect operator++ to return something, not be a void.
• Does block_offset really need to be public?
• On that same note, do your actual concrete implementations of the iterators need to be public, since you have methods to create them that are public? Would it make sense for anyone to ever want to create them a different way?

Answer 2

There is a memory leak in Grid::row_iter(), et al. Why use new in this case? I'd prefer

RowIterator row_iter(int row){return RowIterator(this, row);}