Which block of code is 'better'?

Question

In order to promote good programming habits and increase the efficiency of my code (Read: "My brother and I are arguing over some code"), I propose this question to experienced programmers:

Which block of code is "better"? For those who can't be bothered to read the code, is it worth putting a conditional within a for-loop to decrease the amount of redundant code than to put it outside and make 2 for-loops? Both pieces of code work, the question is efficiency vs. readability.

    - (NSInteger)eliminateGroup {
            NSMutableArray *blocksToKill = [[NSMutableArray arrayWithCapacity:rowCapacity*rowCapacity] retain];
            NSInteger numOfBlocks = (NSInteger)[self countChargeOfGroup:blocksToKill];
            Block *temp;
            NSInteger chargeTotal = 0;

//Start paying attention here

            if (numOfBlocks > 3) 
                for (NSUInteger i = 0; i < [blocksToKill count]; i++) {
                    temp = (Block *)[blocksToKill objectAtIndex:i];
                    chargeTotal += temp.charge;
                    [temp eliminate];
                    temp.beenCounted = NO;
                }
            }
            else {
                for (NSUInteger i = 0; i < [blocksToKill count]; i++) {
                    temp = (Block *)[blocksToKill objectAtIndex:i];
                    temp.beenCounted = NO;
                }
            }   
            [blocksToKill release];
            return chargeTotal;
        }

Or...

        - (NSInteger)eliminateGroup {
            NSMutableArray *blocksToKill = [[NSMutableArray arrayWithCapacity:rowCapacity*rowCapacity] retain];
            NSInteger numOfBlocks = (NSInteger)[self countChargeOfGroup:blocksToKill];
            Block *temp;
            NSInteger chargeTotal = 0;

//Start paying attention here

            for (NSUInteger i = 0; i < [blocksToKill count]; i++) {
                temp = (Block *)[blocksToKill objectAtIndex:i];
                if (numOfBlocks > 3) {
                    chargeTotal += temp.charge;
                    [temp eliminate];
                }
                temp.beenCounted = NO;
            }
            [blocksToKill release];
            return chargeTotal;
        }

Keep in mind that this is for a game. The method is called anytime the user double-taps the screen and the for loop normally runs anywhere between 1 and 15 iterations, 64 at maximum. I understand that it really doesn't matter that much, this is mainly for helping me understand exactly how costly conditional statements are. (Read: I just want to know if I'm right.)

Answer 1

There is no way to answer this without defining your requirements for "better". Is it runtime efficiency? compiled size? code readability? code maintainability? code portability? code reuseability? algorithmic provability? developer efficiency? (Please leave comments on any popular measurements I've missed.)

Sometimes absolute runtime efficiency is all that matters, but not as often as people generally imagine, as you give a nod towards in your question—but this is at least easy to test! Often it's a mix of all these concerns, and you'll have to make a subjective judgement in the end.

Every answer here is applying a personal mix of these aspects, and people often get into vigorous Holy Wars because everyone's right—in the right circumstance. These approaches are ultimately wrong. The only correct approach is to define what matters to you, and then measure against it.

Answer 2

The first code block is cleaner and more efficient because the check numOfBlocks > 3 is either true or false throughout the whole iteration.

The second code block avoids code duplication and might therefore pose lesser risk. However, it is conceptually more complicated.

The second block can be improved by adding

bool increaseChargeTotal = (numOfBlocks > 3)

before the loop and then using this boolean variable instead of the actual check inside the loop, emphasizing the fact that during the iteration it doesn't change.

Personally, in this case I would vote for the first option (duplicated loops) because the loop bodies are small and this shows clearly that the condition is external to the loop; also, it's more efficient and might fit the pattern "make the common case fast".

Answer 3

All other things being equal, having two separate loops will generally be faster, because you do the test once instead of every iteration of the loop. The branch inside the loop each iteration will often slow you down significantly due to pipeline stalls and branch mispredictions; however, since the branch always goes the same way, the CPU will almost certainly predict the branch correctly for every iteration except for the first few, assuming you're using a CPU with branch prediction (I'm not sure if the ARM chip used in the iPhone has a branch predictor unit).

However, another thing to consider is code size: the two loops approach generates a lot more code, especially if the rest of the body of the loop is large. Not only does this increase the size of your program's object code, but it also hurts your instruction cache performance -- you'll get a lot more cache misses.

All things considered, unless the code is a significant bottleneck in your application, I would go with the branch inside of the loop, as it leads to clearer code, and it doesn't violate the don't repeat yourself principle. If you make a change to once of the loops and forget to change the other loop in the two-loops version, you're in for a world of hurt.

Answer 4

I would go with the second option. If all of the logic in the loop was completely different, then it would make sense to make 2 for loops, but the case is that some of the logic is the same, and some is additional based upon the conditional. So the second option is cleaner.

The first option would be faster, but marginally so, and I would only use it if I found there to be a bottleneck there.

Answer 5

You would probably waste more time in the pointless and unnessesary [blocksToKill retain]/[blocksToKill release] at the start/end of the method than the time taken to execute a few dozens comparisons. There is no need to retain the array since you wont need it after you return and it will never be cleaned up before then.

IMHO, code duplication is a leading cause of bugs which should be avoided whenever possible.

Adding Jens recomendation to use fast enumeration and Antti's recomendation to use a clearly named boolean, you'd get something like:

    - (NSInteger)eliminateGroup {
        NSMutableArray *blocksToKill = [NSMutableArray arrayWithCapacity:rowCapacity*rowCapacity];
        NSInteger numOfBlocks = (NSInteger)[self countChargeOfGroup:blocksToKill];
        NSInteger chargeTotal = 0;

        BOOL calculateAndEliminateBlocks = (numOfBlocks > 3);
        for (Block* block in blocksToKill) {
            if (calculateAndEliminateBlocks) {
                chargeTotal += block.charge;
                [block eliminate];
            }
            block.beenCounted = NO;
        }
        return chargeTotal;
    }

If you finish your project and if your program is not running fast enough (two big ifs), then you can profile it and find the hotspots and then you can determine whether the few microseconds you spend contemplating that branch is worth thinking about — certainly it is not worth thinking about at all now, which means that the only consideration is which is more readable/maintainable.

Answer 6

My vote is strongly in favor of the second block.

The second block makes clear what the difference in logic is, and shares the same looping structure. It is both more readable and more maintainable.

The first block is an example of premature optimization.

As for using a bool to "save" all those LTE comparisons--in this case, I don't think it will help, the machine language will likely require exactly the same number and size of instructions.

Answer 7

The overhead of the "if" test is a handful of CPU instructions; way less than a microsecond. Unless you think the loop is going to run hundreds of thousands of times in response to user input, that's just lost in the noise. So I would go with the second solution because the code is both smaller and easier to understand.

In either case, though, I would change the loop to be

for (temp in blocksToKill) { ... }

This is both cleaner-reading and considerably faster than manually getting each element of the array.

Answer 8

Readability (and thus maintainability) can and should be sacrificed in the name of performance, but when, and only when, it's been determined that performance is an issue.

The second block is more readable, and unless/until speed is an issue, it is better (in my opinion). During testing for your app, if you find out this loop is responsible for unacceptable performance, then by all means, seek to make it faster, even if it becomes harder to maintain. But don't do it until you have to.