Is storing iterators inside this class unwise? How else to iterate through this sequence?

Warning this is a long question!

I am implementing a Solitaire card game in C++ on Win32, and after asking this question, it's becoming clear that I may need a bit of guidance regarding actual class design rather than implementation details.

I am using a model view Controller pattern to implement the game. The model is the game, cards, columns and move history. The view is responsible for painting to screen and the control responsible for handling messages and the timer.

The aspect of the game that I am currently trying to implement is the Action history, which belongs to the Model - I want to be able to "undo" and "redo" any Action.

I describe a Move object as an atomic move of a sin开发者_StackOverflow社区gle card from one CardPile to another. And I described an Action as consisting of one or more Moves. e.g. a deal will be 10 Moves from the Deck to a particular Column. ( Deck and Column are simply specializations of CardPile).

I define the Action as a deque of Moves, and have provided some functions to GetCurrentMove() and to Advance() a move when it has been performed.

class Action
{
 public:
    void SetMoves( std::deque<Move> dmoves){ _m_deque = dmoves; }
    void Advance();
    std::deque<Move>::const_iterator GetCurrentMove();
 private:
    std::deque<Move> _m_deque;
    std::deque<Move>::const_iterator currentmove;
};

When dealing (or setting up, or undoing), these Move objects are data for an animation. I need to be able to access a single Move object at a time. I retrieve the Move, parse it into x,y co-ords and then kick off an animation to move a card from one place to another on screen. When the card has reached its destination, I then pull another Move from the deque.

I have been advised by others with more experience, not to store iterators inside the Action class. The STL doesn't do this and there are good reasons apparently.

But my question is - don't I have to store iterators inside the Action class? You see both my Model and View need access to the current Move, so where else can I store that iterator that refers to the current Move ... inside the Controller?

My game animation is based (very broadly) on this model:

void control.game_loop( message )
{
   switch( message )
   {
      case TIMER:
      {
          if( view.CardHasReachedDestination() )
          {
             game.AdvanceAnimation();
             if( !game.AnimationFinished() )
                 view.Parse(game.GetNextMove());
          }
          view.MoveCardXY();
          view.PaintToTheScreen();
          controller.StartTheTimerAgain();
      }
   }
}

Best wishes,

BeeBand

I would create a function template to animate an entire Action, not just one Move at a time. Invoke that with beginning and ending iterators for the Action that needs to be animated:

template <class iterator>
void animate_action(iterator first, iterator last) { 
    for (iterator i=first; i!=last; ++i)
        animate_move(*i);
}

Where animate_move is pretty much what you already had for showing the animation of a single move. You'd invoke this with something like:

animate_action(action.begin(), action.end());

or to animate in reverse order:

animate_action(action.rbegin(), action.rend());

This is (a large part of) why you want to make animate_action a template -- this way it neither knows nor cares whether it receives a forward iterator or a reverse_iterator.

Edit: Based on the further comments, there seem to be a few alternatives.

The standard stand-by would be to use a separate thread to handle the animated drawing, so it would just have something like:

while (current_position != final_position) {
    draw_card(currrent_position);
    current_position = next_position();
    sleep(timer_period);
}

Another would be rather than waiting for a timer to fire, and requesting the current iterator at that point, I'd tend to create and queue up an object representing each move in the animation, then when the timer fires, the timer function retrieves and executes the next item in the queue:

for (int i=0;i<Move.size(); i++)
    for (int j=0; j<num_positions; j++)
       enqueue(move, Move[i], position(j));

If you use a list instead of a deque, then the iterators pointing to an element will remain valid for the lifetime of the element.

If you do that, I don't see anything wrong with storing iterators in this case.