Skip to content

Conversation

nepsilon
Copy link
Contributor

@nepsilon nepsilon commented Sep 5, 2013

An advantage of CoffeeScript compared to JavaScript is its ability to
check for array membership with the in keyword.
A feature it borrowed from Python.

I thought it could be a good idea to use it whenever possible, as it draws a clear difference between the 2 languages, showcasing a convenient CoffeeScript feature.

An advantage of CoffeeScript compared to JavaScript is its ability to
check for array membership with the `in` keyword.
A feature it borrowed from Python.
Copy link
Member

Choose a reason for hiding this comment

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

Good change. Can you put the array in a semantically-meaningful variable? For example

  successResultCodes = [200, 304]
  if req.status in successResultsCodes

Then it reads even more naturally. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.
I just commited this change.

Julien
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Thursday, September 5, 2013 at 9:17 PM, James Holder wrote:

In chapters/ajax/ajax_request_without_jquery.md (http://ajax_request_without_jquery.md):

@@ -47,7 +47,7 @@ loadDataFromServer = -> > > req.addEventListener 'readystatechange', -> > if req.readyState is 4 # ReadyState Compelte > - if req.status is 200 or req.status is 304 # Success result codes > + if req.status in [200, 304] # Success result codes
Good change. Can you put the array in a semantically-meaningful variable? For example
successResultCodes = [200, 304] if req.status in successResultsCodes

Then it reads even more naturally. :)


Reply to this email directly or view it on GitHub (https://github.com/coffeescript-cookbook/coffeescript-cookbook.github.com/pull/89/files#r6180754).

Copy link
Member

Choose a reason for hiding this comment

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

Glad to help!

On Thu, Sep 5, 2013 at 9:33 AM, nepsilon notifications@github.com wrote:

In chapters/ajax/ajax_request_without_jquery.md:

@@ -47,7 +47,7 @@ loadDataFromServer = ->

req.addEventListener 'readystatechange', ->
if req.readyState is 4 # ReadyState Compelte

  •  if req.status is 200 or req.status is 304   # Success result codes
    
  •  if req.status in [200, 304]   # Success result codes
    

Good idea. I just commited this change.
… <#140ee54a1c0a3a35_>
-- Julien Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
On Thursday, September 5, 2013 at 9:17 PM, James Holder wrote: In
chapters/ajax/ajax_request_without_jquery.md (
http://ajax_request_without_jquery.md): > @@ -47,7 +47,7 @@
loadDataFromServer = -> > > req.addEventListener 'readystatechange', -> >
if req.readyState is 4 # ReadyState Compelte > - if req.status is 200 or
req.status is 304 # Success result codes > + if req.status in [200, 304] #
Success result codes Good change. Can you put the array in a
semantically-meaningful variable? For example successResultCodes = [200,
304] if req.status in successResultsCodes Then it reads even more
naturally. :) — Reply to this email directly or view it on GitHub (
https://github.com/coffeescript-cookbook/coffeescript-cookbook.github.com/pull/89/files#r6180754).


Reply to this email directly or view it on GitHubhttps://github.com//pull/89/files#r6181142
.

@sukima
Copy link
Contributor

sukima commented Sep 6, 2013

I like this. Anyone object to a :shipit: ?

@peterhellberg
Copy link
Member

Looks good to me :)

:shipit:

sukima added a commit that referenced this pull request Sep 9, 2013
Bring CoffeeScript idiom to conditional statement.
@sukima sukima merged commit bef9139 into coffeescript-cookbook:master Sep 9, 2013
@nepsilon nepsilon deleted the better-coffeescript-idiom branch September 14, 2013 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants