Sometime this week I was tasked with implementing some super-secret whiz-bang moderation tool for the Elf Island mod team. Since we’re big fans of code reuse (we have one hell of a dev team..you should have seen the code base when we inherited it 7 months ago…), I decided to piggy back off some functionality we added a few weeks earlier that supplied room list information to the client based on some filter parameters. The old code looked something like this:
for each (room in currentRooms) {
...
//Filter based on params
if (privateParam != room.isPrivate || room.isLimbo) continue;
...
}
This was clever because it let us filter one of two disjoint subsets in a single line given 3 parameters. Then came the natural extension (..oof..) of adding another param to get both sets for this whiz-bang feature.
for each (room in currentRooms) {
...
if (!allParam && (privateParam != room.isPrivate || room.isLimbo))
continue;
...
}
At first glance, this looked sensible. Unfortunately, it was brittle and fell apart when some “other” brittle code (that we did not control) did what average java developers do best (assume stupid things, eat exceptions, then throw nonsensical messages into logs). The issue was two things: the obvious order of operations issue and the larger problem of lack of intent expression. Sure it was fun to look at but it’s not easy to maintain. Had I refactored this into a separate method with a clearly defined flow, I wouldn’t have spent hours chasing my tail due to the aforementioned foolish error in the logs. The issue only cropped up under load (when the chance was ~100% a user was in a “limbo” room). The lesson to take from this post is to urge people to write readable code, even if it’s a tiny bit more verbose, instead of attempting to cram things like a 4 parameter filter with weird edge cases into a single line. Code should express intent and it should make sense at first glance, not try to get the job done in the least number of lines possible.