Skip to content

Conversation

oscardelben
Copy link
Contributor

Rationale: easier to understand what's going on, easier to read.

if token == false || !protect_against_forgery?
''
else
if token != false && protect_against_forgery?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this identical to if token &&...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of easy to read code. How about this?

if token.present? && protect_against_forgery?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if token && protect_against_forgery? is fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can receive nil when you don't give an explicit authentication_token, it's actually required to test != false for it to generate one automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have to check if token is false which is different than nil.

@oscardelben
Copy link
Contributor Author

I think we've explored all the edge cases to make a decision about this PR.

rafaelfranca added a commit that referenced this pull request May 12, 2012
@rafaelfranca rafaelfranca merged commit 7be33fe into rails:master May 12, 2012
@rafaelfranca
Copy link
Member

Merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants