Skip to content
This repository
Browse code

gets rid of a double negation, no need to force exactly true/false in…

… a predicate
  • Loading branch information...
commit 0aa66f04e4b4698718023cacb18612e04a4c5eb1 1 parent 11710c0
Xavier Noria authored September 12, 2010
8  actionpack/lib/action_dispatch/http/request.rb
@@ -136,11 +136,11 @@ def content_length
136 136
       super.to_i
137 137
     end
138 138
 
139  
-    # Returns true if the request's "X-Requested-With" header contains
140  
-    # "XMLHttpRequest". (The Prototype Javascript library sends this header with
141  
-    # every Ajax request.)
  139
+    # Returns true if the "X-Requested-With" header contains "XMLHttpRequest"
  140
+    # (case-insensitive). All major JavaScript libraries send this header with
  141
+    # every Ajax request.
142 142
     def xml_http_request?
143  
-      !(@env['HTTP_X_REQUESTED_WITH'] !~ /XMLHttpRequest/i)
  143
+      @env['HTTP_X_REQUESTED_WITH'] =~ /XMLHttpRequest/i
144 144
     end
145 145
     alias :xhr? :xml_http_request?
146 146
 

40 notes on commit 0aa66f0

Tony Arcieri

Let's see how much getting rid of this double negation actually impacts the performance of this method:

https://gist.github.com/2176506

Ruby 1.9.3

      understandable   601539.7 (±14.3%) i/s -    2945866 in   5.016124s (cycle=31339)
           roflscale   705765.9 (±14.9%) i/s -    3444100 in   5.001177s (cycle=34441)

JRuby 1.6.7

      understandable  1887700.7 (±7.9%) i/s -    9378701 in   5.005503s (cycle=36493)
           roflscale  1872240.8 (±6.1%) i/s -    9319996 in   4.999782s (cycle=47551)

Rubinius

      understandable  1502401.4 (±1.3%) i/s -    7528840 in   5.012007s (cycle=31240)
           roflscale  1245542.0 (±1.1%) i/s -    6238034 in   5.008856s (cycle=36058)

This change actually causes a performance degradation on both JRuby and Rubinius, according to my measurements.

It also makes the value returned less clear.

I would strongly support reverting it for the clearer, less confusing return value.

Michael Jackson

The code could be written much more clearly as /XMLHttpRequest/i === @env['HTTP_X_REQUESTED_WITH'], no need to eliminate the double negation to make things pretty. And really, you should return either true or false from ? methods.

Aaron Patterson
Owner

And really, you should return either true or false from ? methods.

why?

Tony Arcieri

why?

Least surprise:

>> request.xml_http_request?
 => true

:thumbsup:

>> request.xml_http_request?
 => 0

o_O

Aaron Patterson
Owner
irb(main):003:0> 1.nonzero?
=> 1
irb(main):004:0>

You test the return value?

Tony Arcieri

Nope, just talking about my perceptions as a user in irb. Sure, 0 is truthy, but is it really a good idea to return it as a truthy value?

IN RUBY, 0 IS TRUE

Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

Yes, it is a magnific idea if you are a Ruby programmer. Matz decides what is true, not you.

Tony Arcieri

@fxn you haven't actually given any rationale as to why you think it's actually a good idea, just endless appeal to authority.

Why is returning 0 instead of true a good idea?

Aaron Patterson
Owner

Sure, 0 is truthy, but is it really a good idea to return it as a truthy value?

Why not? I suspect that if you don't understand that 0 is a truthy value in Ruby, you'll probably learn once you look it up. :trollface:

We're not targeting C programmers with our API. Or are we?

Tony Arcieri

Why not?

At best, I would call it "impolite", especially when you don't have to go out of your way to make a version that returns true/false instead

Michael Jackson

@tenderlove

You test the return value?

nil

Or are we?

0

Aaron Patterson
Owner

Is it OK to return a value that isn't 0, but isn't true?

Aaron Patterson
Owner

@mjijackson Let me get this straight: you test the return value of predicate methods?

Tony Arcieri

@tenderlove I'd say it's... less impolite to return things other than 0 ;)

From the other thread, my list of things that you could return that are better than 0:

  • true
  • 1
  • 42
  • :yes
  • /XMLHttpRequest/
  • "This is indeed an XMLHttpRequest"
Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

@tarcieri I don't appeal to the authority. We are talking about formal languages.

In Ruby 0 is true. That is a fact. Given that fact, the one that has to justify that it is a bad idea that 0 is true is you. And you need to argue with language designers.

Ruby programmers learn the language and use it.

When you do Perl you learn an empty string is false. In Ruby it is not. No problem. They are formal languages, learn them and use them properly.

Aaron Patterson
Owner

@tarcieri sure, but we're ruby programmers and 0 is a truthy value. It seems like this is a bug that you should report upstream from rails.

Tony Arcieri

@fxn programming languages are more than just symbols interpreted by a computer, there's a lot of human-to-human communication that goes on as well. That's why we write programs in Ruby instead of machine language.

Several long-time Rubyists have voiced their opinion that 0 is a confusing "truthy" value regardless of how the Ruby language interprets it. These aren't people who learned 0 is truthy today, they're people who learned that a decade ago, and they're still arguing it's confusing.

You specifically made a change which altered the behavior of this method from returning true/false, which is easy to interpret, to returning 0/nil, and as far as I can tell there was absolutely no rationale as to why this decision was made.

Could you please tell me why 0 is better than true?

Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

@tarcieri I respect the opinion of those Rubyists. And I encourage them to argue with Matz.

On the other hand there are also long time Rubyists like, ahem, me myself and... wait... that guy... ah yes, Matz! which have absolutely no problem with 0 being a true value. Not only that, I love that nil and false are the only false values in Ruby.

So there are programmers with different opinions about this. But I don't care, I have my opinion and see no problem where you see it.

And since I am writing Ruby and you are writing Ruby, I'll be using 0 in my programs to represent a true value if needed with no problem.

What you think of my code, I don't care dude! I am not telling you what you have to do with your code. Go write literals all over the place!

Tony Arcieri

@fxn is there something actually wrong with the version that returned true/false, and if so, why did you feel the need to change it?

There are a lot of truthy values you can return, like :false, and :not_using_xhr, and "nope", but you wouldn't return any of these things because they'd be confusing results as well, even if they are truthy.

0 falls into that same category, imo. true/false are unquestionably clear and beyond reproach.

Oh, by the way, did I mention appeal to authority? Cause you're doing it again

Aaron Patterson
Owner

@tarcieri I thought we were talking about truthiness in Ruby, not whether or not API changed. I'm pretty sure those are two separate topics. :-P

Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

@tarcieri Please see the thread above.

Tony Arcieri

@tenderlove this commit is where the API changed

@fxn o_O if there was an explanation in there somewhere for why this change was a good idea, I missed it

I think I've run out of gas on this discussion, but I'll leave you with this:

From the reaction please realize this is something that bothers a lot of people, and we're not talking about Ruby n00bs, we're talking about hardcore Ruby veterans here. I think they are all well aware of the semantics of truthiness, and Matz's opinions on the matter, and in full knowledge of all these things, they are still bothered.

I humbly ask you get off your high horse for a moment and consider that this sort of change is... impolite, and that 0 is a bad candidate for a truthy value because of its semantic heritage as a fasley value both in other programming languages and in math in general.

Or you can proceed with your crusade to reeducate all the people that zero is the truth...

Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

@tarcieri OK no prob.

The motivation for the change is that since a predicate does not need to return any particular value, there is no point in doing !!. Returning whatever a boolean operator like =~ returns is enough, more simple, and idiomatic.

Another example is

def has_name?
  first_name && last_name
end

The client of that predicate may receive "Smith". No problem, he does not care about the exact value, only its boolean interpretation according to Ruby semantics.

Note performance is not mentioned, performance is not a motivation.

Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

Then the OP was going from Rails 2.3 to 3.1 and had this hash

:ajax_request => request.xhr?

and so that code is relying on the value, while the predicate is just an ordinary Ruby predicate that says nothing in its contract about the way it says that the request is Ajax or not.

Tony Arcieri

@fxn but this can be done while still preserving the true/false return value while still remaining quite clear, as per @tpope's solution. Why did you feel this was so egregious a problem you needed to change the API, and why didn't you look for a solution that returned true/false?

Tony Arcieri

I'll just say the situation was resolved to my satisfaction hours ago. Sorry for making so much noise about it ;)

Xavier Noria
Owner
fxn commented on 0aa66f0 March 23, 2012

@tarcieri I didn't discard @tpope's approach. Doing a good old =~ is perfect! And it is not changing any API. The contract does not guarantee true. In general no Rails predicate says anything about singletons on purpose. The contract is at the boolean level unless something weird requires that the implementation absolutely returns the singletons.

Michael Jackson

@tenderlove No, I don't test the return value of predicate methods. Hence the nil. Perhaps it would've been more clear if I would've said false. :trollface:

But I don't understand what you gain from using String#=~ over Regexp#===. The first means "tell me at what index this regexp matches" and the second means "tell me if this regexp matches this string". When you want to know (as you do in this method) whether or not a Regexp matches a string, I'd suggest you use the method that was built specifically for that purpose. Does that make sense? That way the "you should always return a boolean from a ? method" crowd is happy, you get pretty syntax, and you're more semantically correct. Everybody wins.

Jamie Gaskins

Regardless of which side anyone is on, in at least two of the comment threads on this matter, there has been a lot of "I'm right and you're wrong" attitudes displayed, when it's simply a matter of opinion. Several people on both sides have attacked the other side for having a differing opinion and there is no reason for this. In any argument/debate/discussion, how you present your side has a huge effect on the outcome.

In the original PR, @clifton and @fxn did great at keeping calm and discussing their differing opinions like civilized individuals. I really do applaud them for it. Unfortunately, the discussion went downhill as soon as people began writing incendiary comments toward @fxn for having a different opinion. If I were in his position and I were on the fence about the idea, as soon as the insults began rolling in, I would very likely have jumped to the defensive and closed the issue for that reason alone.

It's perfectly fine for him to have a different opinion on the matter. In the end, this project belongs to Rails core, so they can have whatever code in it they like. And sure, you're free to criticize their choices however much you like, but the result of that may just be pissing people off. I'm not sure how fond you are of that idea, but I'm not a fan of angering people over such petty issues. In a community built on love and happiness, we need to treat each other with a lot more respect than we currently do.

Informpro

I am ... Confused. "returns true if ...

Xavier Noria
Owner
fxn commented on 0aa66f0 March 24, 2012

@mjijackson The Ruby standard library documents all return values. That does not mean =~ is an operator whose intention is to compute an offset, the same way && is not an operator whose intention is to compute the value of the leftmost non-false operand.

The semantics of =~ are the ones of a boolean operator, and it is intended to be used like this

if string =~ regexp
  ...
end

Why does the standard library return false and true as nil or integer? Because it can. Because that's a valid way to implement a boolean operator, you need no further justification. Probably the offset is computed and is the more easy thing to return, and it might be of use to someone, who knows. The point is that Ruby true and falsehood are defined as they are, and predicates can and are implemented that way.

Xavier Noria
Owner
fxn commented on 0aa66f0 March 24, 2012

@jgaskins appreciate it.

@tarcieri You might think it is more understandable as far as the return value is concerned but the implementation is a lot more simple and requires less thought, IMO.

Shannon Skipper

If returning true or false was preferred for whatever reason, seems like a ternary might be more readable than a bang on both sides: @env['HTTP_X_REQUESTED_WITH'] =~ /XMLHttpRequest/i ? true : false

Orlando Del Aguila

im pretty sure they are just setting up the stage for a big april fools day prank.. :trollface:

Ary Borenszweig

@fxn are you just bored and looking at how to remove two characters from every method out there?

I mean, what's the regression fixed with this commit? What test was broken?

This makes no sense at all.

Tony Arcieri

I would strongly like to see this change reverted:

  1. The amount of confusion this change created obviously struck a huge nerve with the community
  2. This change broke one of our apps. I won't defend the code in question and I'm not speaking for my employer, but it's a fact. You broke our app because "gets rid of a double negation, no need to force exactly true/false". By saying "you should fix your app to expect a truthy value here" you are telling that to everyone who is trying to upgrade from an earlier version of Rails which has different behavior. There's no fathomable reason why this behavior should've changed, except to make the code microscopically more comprehensible while making the value returned that much more incomprehensible.
  3. true/false makes umpteen bazillion times more sense than 0/nil
  4. Despite everything else, it's slower than both the original version and tpope's version in 634979

From every possible dimension we can possibly analyze this problem, the only defense I see is "double negations are hard to comprehend, let's go shopping". That argument is nothing in the wake of broken apps and broken dreams of Rails users.

I don't want to bust Rails Core's balls here, but please, you are wrong here. Please revert this change. Please Rails Core, keep your community happy and enthusiastic about what you are doing.

My advice to you is to revert this commit and take it one step at a time from there. I'm sure we can find a version of this code that's mutually satisfactory to everyone, but this is obviously not it.

orangeboy

It's like you're so blinded that you don't even read.

  1. A community of ... 4, 5, 10 people?
  2. By making you aware that you shouldn't rely on singletons being returned is itself a good enough reason to change it. Cry more about your broken app and your inadequacies. Just fix the damn app and get on with your life.
  3. I don't agree.
  4. Micro-optimisations don't matter: #5580 (comment) Benchmarks are bullshit: #5580 (comment)

My advice to you is to stop acting like a child and wasting the core team's time with this bullshit. You're gonna cry enough times until they change it or something?

Michael Jackson

@fxn

The Ruby standard library documents all return values.

o_O There are quite a few methods in Ruby's stdlib documentation that don't have any documentation whatsoever.

That does not mean =~ is an operator whose intention is to compute an offset

I'm not talking about the =~ operator in general. To quote myself, I said that String#=~ means "tell me at what index this regexp matches". To quote Ruby's docs:

Match—If obj is a Regexp, use it as a pattern to match against str,and returns the position the match starts, or nil if there is no match. Otherwise, invokes obj.=~, passing str as an argument. The default =~ in Object returns nil.

So, in the case of Request#xml_http_request? the aforementioned obj is indeed a Regexp. And, according to the docs, that means the return value will be the position the match starts in that string. Again, this is not the question we're asking when we say request.xml_http_request?. We're asking whether or not the request was made via XMLHttpRequest, which is a slightly different question than asking at what index our Regexp matches the value of the X-Requested-With header.

Xavier Noria
Owner
fxn commented on 0aa66f0 March 28, 2012

@mjijackson the thread is over, I have disabled notifications, thanks very much.

Please sign in to comment.
Something went wrong with that request. Please try again.