Permalink
Browse files

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

… a predicate
  • Loading branch information...
1 parent 11710c0 commit 0aa66f04e4b4698718023cacb18612e04a4c5eb1 @fxn fxn committed Sep 11, 2010
Showing with 4 additions and 4 deletions.
  1. +4 −4 actionpack/lib/action_dispatch/http/request.rb
@@ -136,11 +136,11 @@ def content_length
super.to_i
end
- # Returns true if the request's "X-Requested-With" header contains
- # "XMLHttpRequest". (The Prototype Javascript library sends this header with
- # every Ajax request.)
+ # Returns true if the "X-Requested-With" header contains "XMLHttpRequest"
+ # (case-insensitive). All major JavaScript libraries send this header with
+ # every Ajax request.
def xml_http_request?
- !(@env['HTTP_X_REQUESTED_WITH'] !~ /XMLHttpRequest/i)
+ @env['HTTP_X_REQUESTED_WITH'] =~ /XMLHttpRequest/i
end
alias :xhr? :xml_http_request?

39 comments on commit 0aa66f0

Contributor

tarcieri replied Mar 23, 2012

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.

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.

Owner

tenderlove replied Mar 24, 2012

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

why?

Contributor

tarcieri replied Mar 24, 2012

why?

Least surprise:

>> request.xml_http_request?
 => true

👍

>> request.xml_http_request?
 => 0

o_O

Owner

tenderlove replied Mar 24, 2012

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

You test the return value?

Contributor

tarcieri replied Mar 24, 2012

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

Owner

fxn replied Mar 24, 2012

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

Contributor

tarcieri replied Mar 24, 2012

@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?

Owner

tenderlove replied Mar 24, 2012

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?

Contributor

tarcieri replied Mar 24, 2012

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

@tenderlove

You test the return value?

nil

Or are we?

0

Owner

tenderlove replied Mar 24, 2012

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

Owner

tenderlove replied Mar 24, 2012

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

Contributor

tarcieri replied Mar 24, 2012

@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"
Owner

fxn replied Mar 24, 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.

Owner

tenderlove replied Mar 24, 2012

@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.

Contributor

tarcieri replied Mar 24, 2012

@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?

Owner

fxn replied Mar 24, 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!

Contributor

tarcieri replied Mar 24, 2012

@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

Owner

tenderlove replied Mar 24, 2012

@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

Owner

fxn replied Mar 24, 2012

@tarcieri Please see the thread above.

Contributor

tarcieri replied Mar 24, 2012

@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...

Owner

fxn replied Mar 24, 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.

Owner

fxn replied Mar 24, 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.

Contributor

tarcieri replied Mar 24, 2012

@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?

Contributor

tarcieri replied Mar 24, 2012

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

Owner

fxn replied Mar 24, 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.

@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.

Contributor

jgaskins replied Mar 24, 2012

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.

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

Owner

fxn replied Mar 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.

Owner

fxn replied Mar 24, 2012

@jgaskins appreciate it.

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

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

@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.

Contributor

tarcieri replied Mar 28, 2012

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.

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?

@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.

Owner

fxn replied Mar 28, 2012

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

Please sign in to comment.