10
\$\begingroup\$

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:
            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;
    }
}
-(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;
}
-(NSMutableSet *) columnsWithChanges {
    NSMutableArray *columns = _board.columns;
    NSMutableSet *columnsWithChanges = [[NSMutableSet alloc]init];
    for (DMColumn *column in columns) {
        for (DMOrb *orb in column.orbs) {
            if (orb.markedForDestruction) {
                [columnsWithChanges addObject:column];
            }
        }
    }
    return columnsWithChanges;
}
-(void) replaceOrbsForRow:(DMRow *)row direction:(MODirection)direction {

    NSMutableArray *markedOrbs = [[NSMutableArray alloc]init];

    if (direction == MODirectionLeft) {

        //get the marked orbs
        for (int i = 0; i < kNumOrbsPerRow; i++) {
            DMOrb *orb = row.orbs[i];
            if (orb.markedForDestruction) {
                [markedOrbs addObject:orb];
            }
        }

        //get the existing orbs in the proper direction
        NSMutableArray *existingOrbs = [[NSMutableArray alloc]init];
        DMOrb *lastMarkedOrb = [markedOrbs lastObject];
        for (int i = lastMarkedOrb.boardPosition.x + 1; i < kNumOrbsPerRow; i++) {
            [existingOrbs addObject:row.orbs[i]];
        }

        //fill up the array to minimum necessary size with new orbs
        while (existingOrbs.count < markedOrbs.count) {
            [existingOrbs addObject:[DMOrb randomOrb]];
        }

        //set the marked orbs to the appropriate orbs
        int orbIndex = 0;
        DMOrb *firstMarkedOrb = [markedOrbs firstObject];
        for (int i = firstMarkedOrb.boardPosition.x; i < firstMarkedOrb.boardPosition.x + markedOrbs.count; i++) {
            [row setOrbInColumn:i toOrb:existingOrbs[orbIndex]];
            orbIndex++;
        }

        //set remaining existing orbs to their changed positions
        //set new orbs for empty spaces at the end
        for (int i = firstMarkedOrb.boardPosition.x + markedOrbs.count; i < kNumOrbsPerRow; i++) {
            if (orbIndex < existingOrbs.count) {
                [row setOrbInColumn:i toOrb:existingOrbs[orbIndex]];
                orbIndex++;
            } else {
                [row setOrbInColumn:i toOrb:[DMOrb randomOrb]];
            }
        }

    } else if (direction == MODirectionRight) {

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

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

        //fill up the array to minimum necessary size with new orbs
        while (existingOrbs.count < markedOrbs.count) {
            [existingOrbs addObject:[DMOrb randomOrb]];
        }

        //set the marked orbs to the appropriate orbs
        int orbIndex = 0;
        DMOrb *firstMarkedOrb = [markedOrbs firstObject];
        for (int i = firstMarkedOrb.boardPosition.x; i > firstMarkedOrb.boardPosition.x - markedOrbs.count; i--) {
            [row setOrbInColumn:i toOrb:existingOrbs[orbIndex]];
            orbIndex++;
        }

        //set remaining existing orbs to their changed positions
        //set new orbs for empty spaces at the end
        for (int i = firstMarkedOrb.boardPosition.x - markedOrbs.count; i >= 0; i--) {
            if (orbIndex < existingOrbs.count) {
                [row setOrbInColumn:i toOrb:existingOrbs[orbIndex]];
                orbIndex++;
            } else {
                [row setOrbInColumn:i toOrb:[DMOrb randomOrb]];
            }
        }
    }
}
-(void) replaceOrbsForColumn:(DMColumn *)column direction:(MODirection)direction {

    NSMutableArray *markedOrbs = [[NSMutableArray alloc]init];

    if (direction == MODirectionUp) {

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

        //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]];
        }

        //fill up the array to minimum necessary size with new orbs
        while (existingOrbs.count < markedOrbs.count) {
            [existingOrbs addObject:[DMOrb randomOrb]];
        }

        //set the marked orbs to the appropriate orbs
        int orbIndex = 0;
        DMOrb *firstMarkedOrb = [markedOrbs firstObject];
        for (int i = firstMarkedOrb.boardPosition.y; i > firstMarkedOrb.boardPosition.y - markedOrbs.count; i--) {
            DMRow *row = _board.rows[i];
            [row setOrbInColumn:column.columnNumber toOrb:existingOrbs[orbIndex]];
            orbIndex++;
        }

        //set remaining existing orbs to their changed positions
        //set new orbs for empty spaces at the end
        for (int i = firstMarkedOrb.boardPosition.y - markedOrbs.count; i >= 0; i--) {
            DMRow *row = _board.rows[i];
            if (orbIndex < existingOrbs.count) {
                [row setOrbInColumn:column.columnNumber toOrb:existingOrbs[orbIndex]];
                orbIndex++;
            } else {
                [row setOrbInColumn:column.columnNumber toOrb:[DMOrb randomOrb]];
            }
        }

    } else if (direction == MODirectionDown) {

        //get the marked orbs
        for (int i = 0; i < kNumOrbsPerRow; i++) {
            DMOrb *orb = column.orbs[i];
            if (orb.markedForDestruction) {
                [markedOrbs addObject:orb];
            }
        }

        //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 < kNumOrbsPerRow; i++) {
            [existingOrbs addObject:column.orbs[i]];
        }

        //fill up the array to minimum necessary size with new orbs
        while (existingOrbs.count < markedOrbs.count) {
            [existingOrbs addObject:[DMOrb randomOrb]];
        }

        //set the marked orbs to the appropriate orbs
        int orbIndex = 0;
        DMOrb *firstMarkedOrb = [markedOrbs firstObject];
        for (int i = firstMarkedOrb.boardPosition.y; i < firstMarkedOrb.boardPosition.y + markedOrbs.count; i++) {
            DMRow *row = _board.rows[i];
            [row setOrbInColumn:column.columnNumber toOrb:existingOrbs[orbIndex]];
            orbIndex++;
        }

        //set remaining existing orbs to their changed positions
        //set new orbs for empty spaces at the end
        for (int i = firstMarkedOrb.boardPosition.y + markedOrbs.count; i < kNumOrbsPerRow; i++) {
            DMRow *row = _board.rows[i];
            if (orbIndex < existingOrbs.count) {
                [row setOrbInColumn:column.columnNumber toOrb:existingOrbs[orbIndex]];
                orbIndex++;
            } else {
                [row setOrbInColumn:column.columnNumber toOrb:[DMOrb randomOrb]];
            }
        }
    }
}
\$\endgroup\$
8
\$\begingroup\$
-(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]];
}

Aren't we going through the same loop twice for no reason?

Can't we just do this:

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

Followed by:

while (existingOrbs.count < kNumOrbsPerRow) {
    [existingOrbs addObject:[DMOrb randomOrb]];
}

And this should eliminate the next for loop where we're setting positions, I believe. But for good measure, I do want to point something out:

for (int i = firstMarkedOrb.boardPosition.y; i < firstMarkedOrb.boardPosition.y + markedOrbs.count; i++) {
    DMRow *row = _board.rows[i];
    [row setOrbInColumn:column.columnNumber toOrb:existingOrbs[orbIndex]];
    orbIndex++;
}

The orbIndex++; within the loop body isn't particularly good. You should know that we can put this into the update statement. The following is perfectly valid C/ObjC syntax:

for (int i = someCalculatedValue, orbIndex = 0; i < someUpperLimit; ++i, ++orbIndex) {
    // do loop stuff
}

And it's more readable. If the declaration/conditional/update statements in your for statement are too long, separate them onto multiple lines:

for (
    int i = someCalculatedValue, orbIndex = 0;
    i < someUpperLimit;
    ++i, ++orbIndex
) {
  // do loop stuff  
}
\$\endgroup\$
8
\$\begingroup\$

So, I wanted to post another answer. I think we can simplify the left-vs-right logic.

The difference in left versus right is whether we count forward or backward, correct? Presuming some of the stuff in my other answer that already simplifies your code is accurate, I wanted to post an example of using Objective-C blocks to simplify your code even further.

This specific snippet is what I'm assuming is accurate:

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

And the difference is between left and right is whether we iterate forward or reverse, but inside the loop, the logic is the same either way.

So, let's create a block.

NSMutableArray *markedOrbs = [NSMutableArray array];
NSMutableArray *existingOrbs = [NSMutableArray array];

void (^handleOrb)(DMOrb *) = ^(DMOrb *orb) {
    if (orb.markedForDestruction) {
        [markedOrbs addObject:orb];
    } else {
        [existingOrbs addObject:orb];
    }
};

Now, we if on the direction and execute this block in an appropriate loop:

if (direction == MODirectionLeft) {
    for (DMOrb *orb in column.orbs) {
        handleOrb(orb);
    }
} else {
    for (DMOrb *orb in [column.orbs reverseObjectEnumerator]) {
        handleOrb(orb);
    }
}

I assume the if/else is clear enough. I want to clarify what's going on with the block though.

Most Objective-C programmers are comfortable with blocks that take no arguments and return no values:

[object someMethodWithCompletion: ^{
    // this is a void-void completion block
}];

And occasionally, we might see a block that takes an argument:

[object someMethodWithBoolCompletion: ^(BOOL success) {
    if (success) doStuff();
}];

Both of these are anonymous one-time use blocks that are only used by whatever method we're passing them to.

But here, we've created a reusable, named block.

So it takes many parts. First, the declaration:

void (^handleOrb)(DMOrb *);

This is similar to doing something like:

int myInt;

We've declared a variable (and not given it a value). In the case of the int, our variable's type is int and its name is myInt. In the case of our block, our variable's type is a block that takes a DMOrb * and returns void. Our variable's name is handleOrb.

Next, we set it's value.

handleOrb = ^(DMOrb *) {
    // stuff
};

And now, handleOrb can be called just like any other C-function. But it can also be passed around as a variable (which is what we're doing with our anonymous block parameters). An Objective-C block is similar to having a pointer to a C-function.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.