Variables for clarification
Before:
if (![newTrackURL isEqualToString:trackURL] || ([newTrackURL hasPrefix:@"http://"] && !([lastPostedDescription isEqualToString:displayString]))) { // this is different from previous notification, or it's a stream and the playing track has changed
After:
BOOL URLChanged = ![trackURL isEqualToString:newTrackURL];
BOOL isStream = [newTrackURL hasPrefix:@"http://"];
BOOL descriptionChanged = ![lastPostedDescription isEqualToString:displayString];
if (URLChanged || (isStream && descriptionChanged)) {
Why remove the comment? Well, I had just changed the code to what you see in the Before example, but I had not changed the comment, and I thought I was ready to commit. I almost committed a stale comment. It’s that easy. Luckily, I caught it in reading the patch before I qfinished it, so I was able to change the comment to what you see above.
Now I have no comment at all, but the code is clear, so I don’t need one. This is an improvement.
August 14th, 2009 at 11:37:04
There’s a real art to which temporary variables you use. In addition to being self-documenting now, you’ll also find it much easier to debug. A breakpoint on the if line will give you access to the outcomes of each of the subconditions.
This is something that a lot of Objective-C programmers wouldn’t do. I’ve flip-flipped on this. The idea of temporaries, particularily when sending messages to them, seems counter to the language flow.
August 14th, 2009 at 12:35:07
So is the After code what the Before looked like at first? What was the original code and was the comment there?
I agree the second set of code is much easier to read and see what’s being checked and probably doesn’t need a comment.
August 14th, 2009 at 14:50:28
Patrick Burleson:
Huh? Then it wouldn’t be After, would it?
The code went through four generations, two of which exist in version control:
August 14th, 2009 at 21:01:39
One of the questions I always ask myself when I am looking at my own code like this is what will the compiler actually do with this? I honestly don’t know the answer, but it is possible that both of them will come out with the same resultant binaries. Anyone know the answer?
August 16th, 2009 at 02:56:11
@Karl: here you have a reasonably modern optimizing compiler (based on gcc or llvm), which can do a lot, so one could expect that the compiler will do a good job. However your question remains pertinent, because the if clause, compatible with C, has lazy evaluation semantics, i.e. in C it is 100% legal to write things such as:
int foo(bar_t *mybar) {
if(mybar != NULL && mybar->baz(quux))
…
}
The second test will only be done if the first test succeeds, and this is very important if the second test has side effects. Some code conventions forbid any function call in an if clause because side effects can be hard for the programmer to predict.
So in the original, complex version of the test in this blog entry, the generated code would only perform the tests as necessary. In the second “improved” version, the tests can only be optimized away if the compiler can be sure that 1) their results are not needed later on (this is a common optimization), and 2) they have no side effect.
All in all, I really suspect the compiler can only produce the same code if the object methods used above are in header files. But of course, you should only care about this kind of detail if you are looking at a hot spot in your code.