Jay Paroline

One of my personal pet peeves is code that contains a bunch of conditional logic to handle seemingly special cases that really aren’t.

For example, let’s say we have a page that shows a user their playlists if they are logged in, but this page also has other useful information on it, and users aren’t required to log in. In our original design for our authentication class, if authentication “failed” (i.e. the user was not logged in), a call to Auth::getUserID() would return an empty string.

With that implementation, every piece that might have something to display to a logged in user has to have a bunch of conditional logic checking to see if the user is logged in, doing one thing i they are and another if they are not, adding unnecessary complexity and, of course, more potential for bugs to crop up.

This type of logic is completely unnecessary. I changed Auth to return a userID of 0 if the user is not logged in, and now it is not necessary to have any special handling for that case. If the user is logged in, they get playlists. If the user is not logged in, userID 0 does not have any playlists so they do not get playlists. I estimate that this simple change made over 100 lines of code obsolete. Not a whole lot of code in the grand scheme of things, but how many bugs can hide in 100 lines of code?

Another example of the same principle involves exceptions (which, as regular readers of Raymond Chen know, are hard to deal with). Under certain circumstances, recommendations from an external source can be included with our normal set of recommendations. The author of the recommendation-supplementing code originally had it throwing exceptions whenever it had problems. Of course, the case of not having supplemental recommendations because of an error is not really a special case. As far as my code is concerned, you just don’t have supplemental recommendations. In this case modifying the original code eliminates unnecessary handling for a special case and eliminates the potential for an uncaught exception to slip through.

A general rule of thumb to help avoid unnecessary special cases, at least with PHP, is to always return what you say you are going to return (and don’t throw exceptions). If your method processes some data which results in an array, return an array even if the processing has no results. This is the way most native methods in PHP already work, if you think about it. count(array()) doesn’t return an empty string or null or raise an exception, nor does count(null). Really, neither of these are special cases. In either case, the number of elements is zero, and developers are not required to care about the differences.

After some poking around and scratching our heads a bit, Katy and I were able to discover the source of the problem. Turns out what I hit is a compiler/language/optimization bug. Katy has more about it here, including the generic code we were able to distill it down to in order to cause the breakage.

We thought it was a permutation of this bug which is fixed, but apparently this is a new one, so we are going to file it.

I find it a bit disturbing that the related bug was brought to their attention in 2006 and the limited fix was only released to the general public 2 months ago. Over two years to release a fix to a language bug?!? From the comments on the bug, they do not seem to take it very seriously:

Fixed in the avmplus mainline. I’d suggest a 10.0 target date, since the change will need soaking time, there is a work-around and it is rare to encounter this problem.

Change 243008 by rreitmai@rickr-dev on 2006/09/08 12:37:23

Severity: important
Summary: Verifier error bugfix

Detailed Description:

An optimization in the verifier allows us to avoid checking for null in various circumstances. Unfortunately, we were being a little too aggressive and we missed a case. If a block is a target of a backwards branch then we need to assume upon entry of the block that no check has been performed.

The other way to fix this would be to emit null checks just prior to the branch for any values that have notNull true, but this could also create a bunch of unnecessary checks.

Aren’t obscure language bugs the absolute worst kind to have? In my case, it first went undetected because it didn’t trigger any errors/warnings with the debug version of flash (although our sample condensed code does), and then it was extremely difficult to track down because THE CODE WAS CORRECT, and when the production version of flash fails, it does so silently.

I am a PHP and MySQL developer, primarily, and my main responsibility is the server-side Grooveshark Lite components. A couple of days ago I got to write my first ever Actionscript for the Grooveshark Lite front end; an ad rotator to handle ads more statefully and therefore more intelligently.

First I ran into a horrible language bug that causes a nasty runtime crash wherein Flash complains that it can’t resolve a class with itself. I wasn’t doing any weird voodoo with class definitions and Katy, our primary Flex developer, couldn’t find anything wrong with my code, but rewrote some of it anyway. Poof, problem fixed.

Everything looked fine so we thought we were ready to launch. So we did last night. Skyler complained that album art wasn’t loading for him, but he had just reinstalled 64 bit flash on 64 bit Linux, and he’s had weird problems with that before. We were unable to reproduce, attributed it to Flash+Linux weirdness and ignored the issue. Today, more people complained about it. We were not able to reproduce. Eventually I figured out that the bug was only affecting non-debug versions of Flash, but I couldn’t imagine what would be causing album art to not display: my code has nothing to do with displaying images, and I could see in Firebug that the art was being fetched properly, nothing was wrong with the headers, etc.. But it definitely worked fine in the debug version of Flash. Katy looked at it, and through process of elimination was able to determine that the problem was somewhere in my code, but couldn’t tell where exactly. Everything looked valid, and from the perspective of the debug version of Flash, it was, but my code wasn’t very “actionscripty” – because I’m used to writing PHP and haven’t had any sort of training on actionscript beyond looking at existing code and occasionally looking at a language reference book. Katy couldn’t figure out what was wrong with my code, so she rewrote it, and now everything is back to normal again.

I don’t yet know what part of my code was causing the problem, so on Monday I plan to reinstate my code, downgrade to the non-debug version of flash, and figure out what the heck is causing the bug. My instinct tells me that it’s probably a combination of an optimization setting in non-debug Flash combined with my use of Actionscript in a way that is technically valid but that no Actionscript coder would ever normally use it. For example, Katy says I was using a generic Object as a Dictionary, because I was actually trying to replicate associative arrays in PHP, I didn’t know about the Dictionary object, and when I had asked previously how to do that, I was told to use an Object. :P

The good news is that I don’t think many people outside the company paid much attention to this bug. They were too busy being excited about the new features that Katy introduced: broadcast to twitter, broadcast to facebook, and deep linking support: When you “byte” a song (open the info panel), your URL bar changes. You can simply copy the URL to share with a friend, *and* clicking back and forward work, so if you want to go from the song you opened back to the playlist you found it on, just click back in your browser. Freaky, but it works.

There is an effort underway at GS to modify our framework to make writing code easier than ever before (for the second time). It’s a great idea in theory and the results of what I have seen so far certainly make doing certain things more convenient, which is great when you’re writing code.

But I can’t help but feel that we are perhaps barking up the wrong tree a bit.

Peter Hallam points out that programmers spend most of their time reading code, not writing it. So rather than focusing on making code easier to write, we should be sure that we are making it easy to read, understand and modify. Peter surmises that a 10% reduction in the time it takes to understand code is equivalent to a 100% reduction in the time that it takes to write code. That’s very significant.

One of the mantras I have heard a bit too much is “always favor composition over inheritance.” As Phil Haack points out, composition is great sometimes, but it’s not a perfect design either (because there isn’t one). Personally, I think it’s best to keep composition and inheritance in mind and always prefer whichever one going to lead to easy-to-understand code. Many times I find that to be inheritance. Sometimes the solution is even minor code duplication, such as having each page requiring authentication to do an explicit auth check rather than having the framework infer whether or not an auth check is required based on the name or, and I shudder at the thought, comments in the code.

(and how you handle what has been returned)

Things have been busy at Grooveshark, as usual. These past couple of days I have been hunting a bug both cthulu-like in its scary-strangeness and ninja-like in its stealthy manner. I went through all my code related to this particular project several times with a fine-toothed comb and didn’t catch it until today.

Turns out it wasn’t so strange after all. The fault was definitely mine, but PHP’s quirks certainly didn’t help matters any.
I was using array_search in a straightforward manner, not to find the particular position of an item but to find whether or not the item was in the array at all. The one thing about array_search, especially in the context of PHP’s loose typing, is that if it finds nothing it returns false. Of course, php happily treats false as zero, so how do you check to see if array_search is saying that it wasn’t found, or is saying that it’s the first element in the array? You have to check using strict equivalency, which I remembered, so I wrote my code like this:


$found = array_search('something', $arr);
if ($found === false) {
//handle what you do when it's not in there
}


then, a little while ago, specs changed and there was another case that had to be handled the exact same way as when ‘something’ wasn’t in $arr. So I did this:


$found = array_search('something', $arr);
$found = $found || (SOME_OTHER_CASE);
if ($found === false) {
//handle what you do when it's not in there
}


So in other words, when I went back and looked at that code later, I didn’t notice the triple equals instead of the double, so without much thought I assumed that $found was already a boolean, when it was really only a sometimes-boolean. In a strictly typed language this mistake would, of course, not have been possible. More practically, if array_search returned something other than practically-zero, I would have been able to explicitly handle that special case and store the result of that explicit handling as a boolean. If you read the documentation you will see that array_search actually returned NULL before version 4.2.0. I have to wonder why they decided to change it.

The reason this bug was so hard to find was because it was only a bug when the item being searched was the first item in the array, which it turns out is not that often. By the time I found that bug, there were hundreds of newer lines of code to check first.

Now that the bug is solved and now that I am far into this very technical post I think it’s safe to leak a tiny bit of information about what you, dear user, can expect to see in Grooveshark Lite in the near future: autoplay. We have decided that we want to be your personal DJ. Our tack on this feature aims to get around the chicken and egg problem: how do you build recommendations without user feedback, and how do you get user feedback if you don’t have recommendations to make them want to use the system? I’m not going to answer that question directly, but we hope that you will find the autoplay sessions to be enjoyable, and as you provide feedback to the system, we’ll take that data and make it even better.