Bedazzling My Bejeweled Animations

I've spent a few days working on the animations for my Bejeweled clone and I am pretty happy with the results so I am posting the code for review. This is the most advanced animation code that I have written so far, so there are probably problems with it. Here is a gif of the animation running :

I encountered two major problems when creating this code. The first one was that the update method continually called the animation code for each animation state while it was playing. This could cause weird problems and graphical glitches. I solved this by having a variable keep track of whether an animation was currently playing, and if so it would not run it again. The downside to this is needing to manually set that flag to NO at the end of each animation. The second major problem was that in order to get the animation state to properly advance, I had to place each piece of animation code inside of an SKAction block. I am not sure if this is the proper approach, but it seems to be a good solution.

I will not post the entire scene since it is a lot of code.

First off, I need these instance variables to keep track of certain information at the object level:

//animations
DMGameSceneState _state;
BOOL _isAnimationPlaying;
BZDirection _lastDirection;

Next, here are the conditionals inside of the update method of the Scene:

if (_state == DMGameSceneStatePoppingMatches ||
    _state == DMGameSceneStateReplacingMatches ||
    _state == DMGameSceneStateSwapFailure ||
    _state == DMGameSceneStateSwapSuccess) {
    if (!_isAnimationPlaying) {
        [self processGameAnimationState];
        _isAnimationPlaying = YES;
    }
}

if (_state == DMGameSceneStateRunning || _state == DMGameSceneStateBuildingBoard) {
    [self renderBoardOrbs];
}

Here is the switch statement that controls which animation will play. Initially all of the animation code was contained in this method, but I have since extracted some of it to smaller methods called from insid

_state == DMGameSceneStatePoppingMatches ||
_state == DMGameSceneStateReplacingMatches ||
_state == DMGameSceneStateSwapFailure ||
_state == DMGameSceneStateSwapSuccess

Is _state a bit mask? Is DMGameSceneState an NSOptions?

If so, I'd add to the NSOptions declaration:

DMGameSceneStateShouldUpdate = DMGameSceneStatePoppingMatches |
                               DMGameSceneStateReplacingMatches |
                               DMGameSceneStateSwapFailure |
                               DMGameSceneStateSwapSuccess

And the same can be done for

DMGameSceneStateRunning | DMGameSceneStateBuildingBoard

and these large ||-ed together conditions can simply turn into:

if (_state & DMGameSceneStateShouldUpdate)

outsidePositionForDirection:orb: has some major repetition. (_orbWidth + _orbSpacing) is repeated 8 times. Let's stick that in a variable.

int orbConst = (_orbWidth + _orbSpacing)

And now we can just use that in those 8 spots. And then get rid of your magic -1 and magic 9, and use variables to tell me what those mean.

The biggest thing that bothers me about all of your animation code is how ugly all of this nesting looks.

So, I grabbed the shortest method, and I'm going to use that as an example of how to clean this up.

-(void) playAnimationPoppingMatches {
    [self runAction:[SKAction runBlock:^(void) {
        for (SKSpriteNode *node in _orbLayer.children) {
            for (DMOrb *orb in _game.matchedOrbs) {
                NSString *orbName = NSStringFromCGPoint(orb.boardPosition);
                if ([orbName isEqualToString:node.name]) {
                    [node runAction:[SKAction fadeOutWithDuration:0.2]];
                    break;
                }
            }
        }
        [_game clearMatchedOrbs];
    }] completion:^(void) {
        [self runAction:[SKAction waitForDuration:0.2] completion:^(void) {
            _state = DMGameSceneStateReplacingMatches;
            _isAnimationPlaying = NO;
        }];
    }];
}

First, let's handle the completion block. The completion block itself contains a completion block.

I've not completely wrapped my head around all that your code does, so these are placeholder names and you should find better ones...

First, since we'll be using a void-void block several times and the block syntax is quite ugly, let's typedef a void-void block.

typedef void (^CompletionBlock)(void);

Now, let's start making some block variables!

CompletionBlock block_c = ^(void) {
    _state = DMGameSceneStateReplacingMatches;
    _isAnimationPlaying = NO;
};

CompletionBlock block_b = ^(void) {
    [self runAction:[SKAction waitForDuration:0.2] completion:block_c];
};

CompletionBlock block_a = ^(void) {
    for (SKSpriteNode *node in _orbLayer.children) {
        for (DMOrb *orb in _game.matchedOrbs) {
            NSString *orbName = NSStringFromCGPoint(orb.boardPosition);
            if ([orbName isEqualToString:node.name]) {
                [node runAction:[SKAction fadeOutWithDuration:0.2]];
                break;
            }
        }
    }
    [_game clearMatchedOrbs];
};

Now let's make the only method call the method actually makes:

[self runAction:[SKAction runBlock:block_a] completion:block_b];

Now our code is broken up into distinct chunks that we can more easily digest. Our blocks can more easily be read as they more appropriately should: as discrete chunks of code.

StackExchange Code Review Q#77029, answer score: 4