Finding the Greatest Integer and Ties

Question

I'm trying to find the greatest integer of four integers and its ties. I used the following code. Is there anyway to simplify this?

int winner = MAX(self.playerOneScore, MAX(self.playerTwoScore, MAX(self.playerThreeScore, self.playerFourScore)));

if (winner == self.playerOneScore && winner != self.playerTwoScore && winner != self.playerThreeScore  && winner != self.playerFourScore) {
    NSLog(@"1");
} else if (winner == self.playerTwoScore && winner != self.playerOneScore && winner != self.playerThreeScore && winner != self.playerFourScore) {
    NSLog(@"2");
} else if (winner == self.playerThreeScore && winner != self.playerOneScore && winner != self.playerTwoScore && winner != self.playerFourScore) {
    NSLog(@"3");
} else if (winner == self.playerFourScore && winner != self.playerOneScore && winner != self.playerTwoScore && winner != self.playerThreeScore) {
    NSLog(@"4");
} else if (self.playerOneScore == self.playerTwoScore || self.playerOneScore ==  self.playerThreeScore || self.playerOneScore == self.playerFourScore || self.playerTwoScore == self.playerThreeScore || self.playerTwoScore == self.playerFourScore || self.playerThreeScore == self.playerFourScore) {
    NSLog(@"Tie");
}

Answer 1

Your code looks correct (apart from the typo in case #4). Note that the final else if is not necessary. If none of the first 4 tests passes then you necessarily have a tie, so a simple else is sufficient here.

The winner variable is named badly, as it contains the maximal score and not the winning player.

But you are using far too many comparisons, and the code does not scale well for a larger number of players. What you need to know is

• Which player has the maximum score?
• How many players have the maximum score?

Both can more easily determined if all scores are stored in an array.

Since you are working with Objective-C, I would suggest to take advantage of the classes and methods that the Foundation framework provides.

First create an NSArray of all player scores. Objective-C collections can only contain objects, therefore the integers have to be boxed into NSNumber with @(...):

NSArray *scores = @[@(score1), @(score2), @(score3), @(score4)];

Use Key-Value Coding to find the maximum element:

NSNumber *maxScore = [scores valueForKeyPath:@"@max.self"];
NSLog(@"maximum score is %@", maxScore);

Use indexesOfObjectsPassingTest: to find all objects in the scores array that are equal to the maximum score:

NSIndexSet *bestPlayers = [scores indexesOfObjectsPassingTest:^BOOL(NSNumber *obj, NSUInteger idx, BOOL *stop) {
    return [obj isEqual:maxScore];
}];

If the index set has only one element then there is a unique winner, otherwise a tie:

if ([bestPlayers count] == 1) {
    NSLog(@"Best player is #%ld", [bestPlayers firstIndex] + 1);
} else {
    NSLog(@"Tie between");
    [bestPlayers enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL *stop) {
        NSLog(@"  Player #%ld", idx + 1);
    }];
}

(Array indices are 0-based, therefore you add one to get the player number.)

Answer 2

Martin R already gave you an excellent and comprehensive answer. I cannot find much else wrong with your code, but I will say a couple things.

He already explains how to use an array to greatly simplify the code. I will add that any time you have variables with names like playerOne, playerTwo, playerThree, or maybe something else like vehicle1, and vehicle2, you can be sure that there is a better approach to take. Just having a number after a variable name is never enough information to make its purpose easier to understand. The reader will be left to guess or discover the actual significance of the variable.

The second point I will make is that some of the lines in your code are way too long. Especially when you are doing comparisons, there is a much better way than having long lines like this:

} else if (winner == self.playerFourScore && winner != self.playerOneScore && winner != self.playerTwoScore && winner != self.playerThreeScore) {
} else if (self.playerOneScore == self.playerTwoScore || self.playerOneScore ==  self.playerThreeScore || self.playerOneScore == self.playerFourScore || self.playerTwoScore == self.playerThreeScore || self.playerTwoScore == self.playerFourScore || self.playerThreeScore == self.playerFourScore) {

I would recommend that you do this instead:

} else if (winner == self.playerFourScore 
           && winner != self.playerOneScore 
           && winner != self.playerTwoScore 
           && winner != self.playerThreeScore) {

} else if (self.playerOneScore == self.playerTwoScore 
           || self.playerOneScore ==  self.playerThreeScore 
           || self.playerOneScore == self.playerFourScore 
           || self.playerTwoScore == self.playerThreeScore 
           || self.playerTwoScore == self.playerFourScore 
           || self.playerThreeScore == self.playerFourScore) {

The important thing to note is that all of the comparisons are lined up together. If you are using Xcode, it will handle this alignment for you. Doing comparisons this way will make the code much easier to read. When you are doing multiple comparisons like this the extra lines are definitely worth it. You may not need to use this technique for this specific problem, but it is something to keep in mind for the future.