Keep Those Conditionals Tight

I maintain a small project that has a web-based search engine for searching through a large collection of documents. The results page is fairly simple, it says something like "Found 341 matches in 645,345 documents" and then displays a list of matches. Easy, right?

Well, last weekend my client called me in a panic and said, "It says it found 341 matches, but where the list of documents is supposed to be, it says 'no matches found!'"

Further investigation revealed that this was only happening on some searches, not all of them. Certain search terms would trigger the bug, but not others. It was quite baffling.

I started playing around with the search code, Dumpering various things to the error log to make sure they looked right. They did. I checked the database query logs to make sure the right queries were being executed. They were. I asked the HTML guy if he had changed any of the templates recently, and he said he hadn't, but I decided to check anyway since he's a liar and often forgets to check in his changes before deploying them.

I noticed that the template contained code like this (simplified for brevity):

Found [% numfound %] matches in [% totaldocs %] documents.
<p />
[% IF results.0 %]
  <ul>
    [% FOREACH doc IN results %]
    <li>[% doc %]</li>
    [% END %] 
 </ul>
[% ELSE %]
  No documents found
[% END %]
[download]

So what could cause numfound to be populated correctly and yet make results empty, and only some of the time?

Then it hit me like a sack of bricks. results wasn't empty. It's just that sometimes the first element was a false value. Lo and behold, a document had accidentally been entered into the database with a null title. When that document happened to be in the results set, it of course got sorted first, causing the conditional to fail. Changing the conditional to [% IF results.size > 0 %] fixed that problem right up. (And also demonstrated the redundancy of having a separate numfound variable there. Fixed that also.)

And so ended the reign of the most difficult bug ever for that particular project.

So, the moral of the story is: make sure your conditionals are tight. Test the right thing, and only the right thing.

Comment on Keep Those Conditionals Tight Select or Download Code

Replies are listed 'Best First'.
Re: Keep Those Conditionals Tight by rinceWind (Monsignor) on Mar 10, 2006 at 09:20 UTC
I would expand on this general principle. Keep scope of variables as small as possible. Use `my` rather than `our`, except where needed. The TT equivalent of this is the localization of variables that occurs with INCLUDE and MACRO directives. Look for edge conditions, and build unit tests for them. In Perl code, Devel::Cover can often help here. When writing unit tests for templates, provide test cases with extremes of data, such as your `results` array with an empty first element. -- Oh Lord, won’t you burn me a Knoppix CD ? My friends all rate Windows, I must disagree. Your powers of persuasion will set them all free, So oh Lord, won’t you burn me a Knoppix CD ? (Missquoting Janis Joplin)	[reply] [d/l] [select]
Re: Keep Those Conditionals Tight by dragonchild (Archbishop) on Mar 10, 2006 at 14:04 UTC
Situations like that have gotten me started down the road of testing defined-ness or existence, never truth (except for something that specifically is supposed to be a boolean, usually because it begins with $is_). It's also kinda like the C-programmer's idiom of `if ( 5 == var )` instead of `if ( var == 5 )`. That protects against the accidental `if ( var = 5 )`. My criteria for good software: Does it work? Can someone else come in, make a change, and be reasonably certain no bugs were introduced?	[reply] [d/l] [select]
Re^2: Keep Those Conditionals Tight by xdg (Monsignor) on Mar 10, 2006 at 15:05 UTC
(Nods). Makes me wish there was a `def` as short as `ref`. -xdg Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.	[reply] [d/l] [select]
Re: Keep Those Conditionals Tight by Mutant (Priest) on Mar 10, 2006 at 12:53 UTC
I would also say if you have stupid HTML guys (and don't we all) consider using a templating system that limits the amount of damage they can do, e.g. HTML::Template. I guess you also might ask why are null (or even blank) titles allowed for a document. To me, this is an example of why properly designed databases, including constraints, etc, are so important. It can mean a lot of these sort of problems simply disappearing. And yet many developers (including myself) often skip over things like constraints, or make them wide open just because it's "easier". (Of course, either of these points may not be applicable to your particular project, but they're the sort of things I'd be thinking about if I encountered this type of bug).	[reply]
Re^2: Keep Those Conditionals Tight by friedo (Prior) on Mar 10, 2006 at 15:21 UTC
I would also say if you have stupid HTML guys (and don't we all) consider using a templating system that limits the amount of damage they can do I realize now it wasn't clear from my post, but the bad template logic was written by me, not the dumb HTML guy. I guess you also might ask why are null (or even blank) titles allowed for a document. To me, this is an example of why properly designed databases, including constraints, etc, are so important. Indeed. Unfortunately, in MySQL, when you have a varchar column declared NOT NULL, and you fail to supply a value, it enters an empty string, which is fale in Perl.	[reply]
Re: Keep Those Conditionals Tight by Anonymous Monk on Mar 10, 2006 at 19:33 UTC
You know how old programmers keep saying nasty sounding things like: "Never trust user input?". This is why. They don't mean that users are inherently untrustworthy, just that they'll forget what rules they agreed to abide by, and then the software won't work, and they'll call you in confusion, and then their problem will become your problem. Make your software verify any constraints on user inputs before you assume the input is valid. Suppose the user should enter "T" for true, or "F" for false. Your code should handle the case where someone decides, against all odds, to type "Jello" in that field instead. Because one day, it will happen. -- Ytrew	[reply]