The Revenge of Inigo Montoya

Written by pete

Topics: Uncategorized

:false

You keep using that symbol. I do not think it means what you think it means.

Today, I’d like to describe something I like to call: The Inigo Montoya Attack. It is the implementation in code — often with good intentions, but sometimes simply because of ignorance — of an algorithm which masks one clear, well-known paradigm inside of another that completely obfuscates your actual intent. Unless you have six-fingers on your right hand (I’m talking to YOU, Count Rugen) you are forbidden from this practice.

Here’s some code I came across in a codebase I was maintaining.

 def logged_in_from_token?
    current_user = token_exists? ? log_in_from_token : :false
 end

It looks strange. It OUGHT to look strange. What is that :false symbol all about?

I’ll tell you what it is. It’s a very misleading piece of code. Because when you do not log the user in from a token, current_user is NOT set to false, in the boolean sense. It is, in fact, a boolean “true” value in Ruby.

Try this in IRB:

 if :false  
   puts "You keep using that word. I do not think it means what you think it means." 
else 
  puts "Have fun storming the castle!"
end

If :false evaluated to a boolean false value, then you’d see “Have fun storming the castle!”. But instead, you get “You keep using that word. I do not think it means what you think it means.” If you’re trying to make readable code, using the symbol :false is dangerous and misleading.

To make matters worse, this code assigns :false to the value current_user, which is a well-known and established name used in a number of Rails patterns. RESTFul Authentication and Authlogic BOTH support the notion of current_user, and when nobody is logged in, THAT METHOD RETURNS FALSE. Not the doppleganger wannabe :false, but a real, honest to goodness, boolean false. (OK: it’s actually returning nil, but nil is false).

So imagine my bewilderment when this basic test no longer worked:

  <%- if current_user %>
    <%= display_logged_in_stuff()  %>
  <% else %>
      You are not logged in
   <% end %>

So a couple hours of debugging later, I found myself using ‘ack in project’ to remove all references to the silly symbol :false.

Lessons for Rubyists:

  1. Be clear. :false is never actually false. Don’t use it.
  2. Follow established conventions. The method some_test? should always return a boolean value. If it doesn’t, leave off the question mark!
  3. Don’t reinvent the wheel! The implementor of the code above could have simply used RESTful Authentication or Authlogic and have avoided the whole mess.
  4. Unit tests are still important! Remember that someone has to maintain your stuff someday. A simple unit test would have caught the bug introduced by using the symbol :false in place of a boolean false.

Have fun storming the castle!

  • aaronscruggs

    That is very odd. Do you see it often? I do think it leads to one of my favorite rubyist interview questions: “What puts you in the else block of a conditional?”

  • http://blog.peteonrails.com peteonrails

    I have seen this in exactly one codebase. To protect the innocent, I will not say whose codebase it was, but I will tell you that our friend in common Kate probably knows why the symbol :false is used in place of 'false'.

  • http://blog.peteonrails.com peteonrails

    Other implementations of The Inigo Montoya Attack include cargo-culting things like the bang-bang boolean trick when you don't really need it:

    val = !!false

    Why would you do that? Why?

  • derekharmel

    Yes, :false was a poor choice for a symbol, but if you actually checked the codebase further than just that one line you would've seen that it's never evaluated anywhere for its truthiness.

    Regarding point #3, no we couldn't have “simply” used RESTful Authentication or Authlogic. The latter didn't even exist at the time. I don't know how you can possibly make the assertion that you did. Do you fully understand the requirements of authentication at said company? No, you clearly don't/didn't when you wrote this.

    Regarding point #4, I don't know whether to laugh or be outraged as the project mentioned above has almost no test coverage and the tests that did exist didn't even run when you were done with the project.

    Before you rag on someone else's code and tell them they need tests, make sure that what you wrote for them wasn't a huge steaming pile of untested shit.

  • http://blog.peteonrails.com peteonrails

    I don't usually feed the trolls Derek, especially not on something I
    wrote over a year ago. But in this case, I think a response is
    warranted.

    You're conflating a bunch of issues in your argument, but here's my best go:

    1. Yes, I read all of the code behind ci_auth. The :false symbol may not
    have been evaluated anywhere for its truthiness in the library itself,
    but I assure you, it caused a bug that I spent a long time tracking
    down.

    2. Yes. I understood the requirements of authentication at said company.
    I stand by my assertion that rolling your own security / authentication
    library was a mistake.

    3. There's a reason there are no tests in The Mod. You'll have to ask
    David why, though. I'm really sorry that you're stuck maintaining that
    code base for more reasons than the fact that it's got poor unit test
    coverage.

    In my original post, Derek, I didn't rag on your code. I didn't mention
    your name, your employer, the project in question, or even say it was a
    huge steaming pile of shit. If you think The Mod codebase sucks, then
    you should make the case to your bosses for a rewrite, rather than rag
    on me for making the best of a shitty situation with your stakeholders.

    Just don't put “if case == :false” in there anywhere and I'll be happy.

  • http://blog.peteonrails.com peteonrails

    I don't usually feed the trolls Derek, especially not on something I
    wrote over a year ago. But in this case, I think a response is
    warranted.

    You're conflating a bunch of issues in your argument, but here's my best go:

    1. Yes, I read all of the code behind ci_auth. The :false symbol may not
    have been evaluated anywhere for its truthiness in the library itself,
    but I assure you, it caused a bug that I spent a long time tracking
    down.

    2. Yes. I understood the requirements of authentication at said company.
    I stand by my assertion that rolling your own security / authentication
    library was a mistake.

    3. There's a reason there are no tests in The Mod. You'll have to ask
    David why, though. I'm really sorry that you're stuck maintaining that
    code base for more reasons than the fact that it's got poor unit test
    coverage.

    In my original post, Derek, I didn't rag on your code. I didn't mention
    your name, your employer, the project in question, or even say it was a
    huge steaming pile of shit. If you think The Mod codebase sucks, then
    you should make the case to your bosses for a rewrite, rather than rag
    on me for making the best of a shitty situation with your stakeholders.

    Just don't put “if case == :false” in there anywhere and I'll be happy.