Casey
Casey

Reputation: 10946

Refactoring needed for repetitious switch statements

The many repetitious switch statements seems like it needs to be DRY'd. Any suggestions? (Including doing nothing!)

AnimMapIter _iter;
    _iter = _animations->find(name);
    if(_iter == _animations->end()) return;

    if(_curName != name) {
        _curName = name;

        switch(dir) {
        case DIR_FORWARD_LOOPING: /* Fall through to DIR_FORWARD_NONLOOPING */
        case DIR_FORWARD_NONLOOPING:
            _iter->second->First();
            break;
        case DIR_REVERSE_LOOPING: /* Fall through to DIR_REVERSE_NONLOOPING */
        case DIR_REVERSE_NONLOOPING:
            _iter->second->Last();
            break;
        }
    } else {

        switch(dir) {
        case DIR_FORWARD_LOOPING: /* Fall through to DIR_FORWARD_NONLOOPING */
        case DIR_FORWARD_NONLOOPING:
            _iter->second->Next();
            break;
        case DIR_REVERSE_LOOPING: /* Fall through to DIR_REVERSE_NONLOOPING */
        case DIR_REVERSE_NONLOOPING:
            _iter->second->Previous();
            break;
        }

        switch(dir) {
            case DIR_FORWARD_LOOPING:
                if(_iter->second->IsAtEnd())
                    _iter->second->First();
                break;
            case DIR_FORWARD_NONLOOPING:
                if(_iter->second->IsAtEnd())
                    _iter->second->Last();
                break;
            case DIR_REVERSE_LOOPING:
                if(_iter->second->IsAtFront())
                    _iter->second->Last();
                break;
            case DIR_REVERSE_NONLOOPING:
                if(_iter->second->IsAtFront())
                    _iter->second->First();
                break;
        }
    }

Upvotes: 0

Views: 211

Answers (2)

Kevin Grant
Kevin Grant

Reputation: 5431

Everything under the else should collapse into a single switch to bring the related steps closer; e.g.

case DIR_FORWARD_LOOPING:
    _iter->second->Next();
    if (_iter->second->IsAtEnd()) {
        _iter->second->First();
    }
    break;

...all in that one case. Repetition of a couple of function calls is not a big deal when it makes the overall sequence of actions more clear.

Upvotes: 1

molbdnilo
molbdnilo

Reputation: 66459

Push the logic into whatever _iter->second is, along these lines (assuming the methods you've already shown exist):

class WhateverItIs
{
public:
   void Start() { if (m_forward) First(); else Last(); }
   void Stop()  { if (m_forward) Last(); else First(); }
   void Advance()
   {
      if (m_forward)
         Next();
      else
         Previous();
      if (IsLast())
      {
         if (m_loop)
            Start();
         else
            Stop();
      }
   }

private:
   bool IsLast() const
   {
      return m_forward ? IsAtEnd() : IsAtFront();
   }
   // Direction and looping are independent concepts.    
   bool m_forward;
   bool m_loop;
};

Then you can write:

AnimMapIter _iter;
_iter = _animations->find(name);
if(_iter == _animations->end()) return;

if(_curName != name) {
    _curName = name;
    _iter->second->Start();
} else {
    _iter->second->Advance();
}

Edit: Example using free functions and keeping the constants.

   void Start(Strip* s, bool forward) 
        { if (forward) s->First(); else s->Last(); }
   void Stop(Strip* s, bool forward) 
        { if (forward) s->Last() else s->First(); }
   void Advance(Strip* s, bool forward, bool loop)
   {
      if (forward)
         s->Next();
      else
         s->Previous();
      if (IsLast(s, forward))
      {
         if (loop)
            Start(s);
         else
            Stop(s);
      }
   }

   bool IsLast(const Strip* s, bool forward) const
   {
      return forward ? s->IsAtEnd() : s->IsAtFront();
   } 

   bool Projector::IsForward() const
   { 
       return dir == DIR_FORWARD_LOOPING || dir == DIR_FORWARD_NONLOOPING; 
   }

   bool Projector::IsLooping() const
   {
       return dir == DIR_REVERSE_LOOPING || dir == DIR_FORWARD_LOOPING;
   }

    if(_curName != name) {
        _curName = name;
        Start(_iter->second, IsForward());
    } else {
        Advance(_iter->second, IsForward(), IsLooping());
    }

Upvotes: 1

Related Questions