Refilling a Bejeweled Board

When you start thinking about designing a Match 3 type game, you realize that there are a great many ways to structure the rules. In this particular variant, the direction that the player swipes the orbs determines the way that the board is refilled. The new pieces will enter the board opposite the direction that the player swipes. So if the player swipes down new pieces will enter the board from the top. After the matched pieces are removed, the existing pieces are moved down to replace them, and then new random pieces are generated to replace those existing pieces.

I have tried to think of ways to reduce the redundant code here and also ways to shorten the methods, but I am not sure if it would make the code more readable or not. Left and right swipes are handled differently from up and down swipes, so they need to be in different methods. The logic is similar for each of the four directions but it is distinctly different. I would definitely appreciate feedback on the algorithm I am using.

The first part of this determines whether a match has been made and then marks the orbs for destruction if they are part of a match. You can see that code here in my previous Bejeweled clone question.

This part of the code refills the board after a successful match.

```
#pragma mark - Replace Orbs
-(void) replaceMarkedOrbsForDirection:(MODirection)direction {
NSMutableSet *rowsWithChanges = nil;
NSMutableSet *columnsWithChanges = nil;
switch (direction) {
case MODirectionLeft:
rowsWithChanges = [self rowsWithChanges];
for (DMRow *row in rowsWithChanges) {
[self replaceOrbsForRow:row direction:direction];
}
break;
case MODirectionRight:
rowsWithChanges = [self rowsWithChanges];
for (DMRow *row in rowsWithChanges) {
[self replaceOrbsForRow:row direction:direction];
}
break;
case MODirectionUp:
col

-(void) replaceMarkedOrbsForDirection:(MODirection)direction {
    NSMutableSet *rowsWithChanges = nil;
    NSMutableSet *columnsWithChanges = nil;
    switch (direction) {
        case MODirectionLeft:
            rowsWithChanges = [self rowsWithChanges];
            for (DMRow *row in rowsWithChanges) {
                [self replaceOrbsForRow:row direction:direction];
            }
            break;
        case MODirectionRight:
            rowsWithChanges = [self rowsWithChanges];
            for (DMRow *row in rowsWithChanges) {
                [self replaceOrbsForRow:row direction:direction];
            }
            break;
        case MODirectionUp:
            columnsWithChanges = [self columnsWithChanges];
            for (DMColumn *column in columnsWithChanges) {
                [self replaceOrbsForColumn:column direction:direction];
            }
            break;
        case MODirectionDown:
            columnsWithChanges = [self columnsWithChanges];
            for (DMColumn *column in columnsWithChanges) {
                [self replaceOrbsForColumn:column direction:direction];
            }
            break;
        default:
            break;
    }
}

This can be drastically shorter.

First of all, we don't need the NSMutableSet variables, and second of all, in Objective-C, switch cases fall through by default, thats why we need the break; statement.

- (void)replaceMarkedOrbsForDirection:(MODirection)direction {
    switch (direction) {
        case MODirectionLeft: case MODirectionRight:
            for (DMRow *row in [self rowsWithChanges]) {
                [self replaceOrbsForRow:row direction:direction];
            }
            break;
        case MODirectionUp: case MODirectionDown:
            for (DMRow *row in [self rowsWithChanges]) {
                [self replaceOrbsForRow:row direction:direction];
            }
            break;
    }
}

Also, notice that we don't need the default if we're switching on an enum and handling all cases.

- (NSMutableSet *) rowsWithChanges {
    NSMutableSet *rowsWithChanges = [[NSMutableSet alloc]init];
    for (DMRow *row in _board.rows) {
        for (DMOrb *orb in row.orbs) {
            if (orb.markedForDestruction) {
                [rowsWithChanges addObject:row];
            }
        }
    }
    return rowsWithChanges;
}

We're using a set, which means that adding an object multiple times does nothing. So, let's save some iterations and checks:

- (NSMutableSet *)rowsWithChanges {
    NSMutableSet *rowsWithChanges = [NSMutableSet set];
    for (DMRow *row in _board.rows) {
        for (DMOrb *orb in row.orbs) {
            if (orb.markedForDestruction) {
                [rowsWithChanges addObject:row];
                break;
            }
        }
    }
    return rowsWithChanges;
}

Notice that this break statement only breaks the inner-most loop. Once we know we need to handle this row, we don't need check any more orbs, so let's move on to the next row.

We can do the same with our columnsWithChanges method as well. But note in that method, we can eliminate this line:

NSMutableArray *columns = _board.columns;

And just reference _board.columns directly in the forin loop.

And one final note about these two methods... why don't we declare some readonly properties?

@property (readonly) NSMutableSet *rowsWithChanges;
@property (readonly) NSMutableSet *columnsWithChanges;

No local instance variable is created--there is no _rowsWithChanges or _columnsWithChanges, because we manually implemented each getter already and didn't use the variable. But meanwhile, without changing anything else specifically regarding these properties or methods, we can stop writing:

[self rowsWithChanges]

And instead, use self.rowsWithChanges. Our code will still perform identically but it will read a little nicer.

I think rather than two gigantic methods for left/right or up/down, I'd rather prefer four methods actually--one for each direction. Or--if possible, think of some way to simplify the logic into one method... but yeah...

For some reason, here, we slip out of the forin loops:

for (int i = kNumOrbsPerRow - 1; i >= 0; i--) {
    DMOrb *orb = column.orbs[i];
    if (orb.markedForDestruction) {
        [markedOrbs addObject:orb];
    }
}

Is it important to go backward here? If so, we can still forin backwards:

for (DMOrb *orb in [column.orbs reverseObjectEnumerator]) {
    if (orb.markedForDesctruction) { 
       [markedOrbs addObject:orb];
    }
}

But if backwards isn't important, just don't call reverseObjectEnumerator.

And then I read this next section:

//get the existing orbs in the proper direction
NSMutableArray *existingOrbs = [[NSMutableArray alloc]init];
DMOrb *lastMarkedOrb = [markedOrbs lastObject];
for (int i = lastMarkedOrb.boardPosition.y - 1; i >= 0 ; i--) {
    [existingOrbs addObject:column.orbs[i]];
}

StackExchange Code Review Q#74690, answer score: 8