Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CoffeeScript preferred styles #187

Merged
merged 1 commit into from
Jun 27, 2014
Merged

Conversation

jasonramirez
Copy link
Contributor

Prefer @ over this for referencing instance properties

Prefer double quotes to preserve interpolation, allow for more complex strings, make selectors easier to read and for overall consistency.

  • $("[data-role='title']") or $("input[type='text']")
  • "Isn't this nice?"
  • "It is #{nice}"

@@ -129,6 +129,8 @@ CoffeeScript
private variables and functions.
* Prefer `==` and `!=` to `is` and `isnt`
* Prefer `||` and `&&` to `or` and `and`
* Prefer `@` to `this` for scoping.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "for scoping"? Can you give an example of when we don't prefer @ to this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify this to just read, 'Prefer @ to this'. I don't know of any situation in coffeescript that using this is preferred over @.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've used @ for properties, but not for passing the instance itself. For example:

new Post user: @

I would use this instead of @ there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've used it for passing the instance itself. I got used to it, but I'd really just prefer to have a guideline.

I do think that "Always use @" is a simpler guideline, which has value.

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 'Prefer @ to this' (suggestion from @drapergeek )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabebw the notion to use @ instead of this may be more about context..not scoping. I was trying to state that we should use @ when in reference to the object that 'owns' the currently executing code. This conversation came about when a client asked why we do:

constructor: ->
  @someMethod()

vs.

constructor: ->
  this.someMethod()

So perhaps:
When referencing properties, use @ instead of this
or
Use @ instead of this, unless passing the instance itself.

Copy link

Choose a reason for hiding this comment

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

I agree with Joe. Instead of a broad rule like "use @ anywhere you'd use this", I would prefer something more specific like "Prefer @ over this for referencing instance properties". It honestly looks weird to me to see @ standalone. I've seen this happen generally in two cases:

$(@) (for $(this)):

$('.some_selector').on 'click', ->
  $(@).hide()

@ as the final line of a method (for returning this):

class Foo
  set: (properties) ->
    _.extend this, properties
    @

I have two reasons for using a more specific guideline:

  1. Only using @ for instance attributes and methods feels more like Ruby, so it's less of a context switch.
  2. For a while it was actually discouraged and Jeremy Ashkenas briefly considered removing support for bare @ in CoffeeScript (discussion), but ultimately nothing was done because of the controversy around it. Perhaps a weak point, although, it's been removed in the upcoming "CoffeeScriptRedux".

Copy link

Choose a reason for hiding this comment

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

So yeah, basically what Jason said.

Copy link
Contributor

Choose a reason for hiding this comment

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

Elliot, I like Prefer @ over this for referencing instance properties. It doesn't say anything about using @ or this when passing the instance, but makes it clear when we do care.

That guideline gets my vote.

@kaishin
Copy link

kaishin commented Jun 10, 2014

I use single quotes, except for when I interpolate. Example:

class App.SuggestionListView extends Backbone.View
  ACTIVE_CLASS = 'active'
  SUGGESTION_SELECTOR = '[data-role="suggestion"]'
  updateTranslation: (selectedSuggestion) ->
    $activeSuggestions = $(".#{ACTIVE_CLASS}")

They are twice as fast to type and are easier on the eyes.

I think we should leave this as a per-project matter, just like HAML vs HTML.

@kaishin
Copy link

kaishin commented Jun 10, 2014

Twice as fast -> Require half the effort ;)

@jasonramirez
Copy link
Contributor Author

@kaishin I have no problem leaving it as a per-project matter. I was looking through the CoffeeScript in our past projects, and we've done a mix of both versions of quoting, so I was hoping to get some preference for consistency. Something like (i think) we do in Ruby, where we use single quotes unless we have to use double quotes, seems like where we are.

I will put this part to bed if nobody finds value in setting a preference.

No Preference:

a = 'They Are'
b = "They're"
c = "[data-role='suggestion']"
d = '[data-role="content"]'
e = "#{whoops}"

Double Quote Preference:

a = "They Are"
b = "They're"
c = "[data-role='suggestion']"
d = "[data-role='content']"
e = "#{whoops}"

@mcmire
Copy link

mcmire commented Jun 13, 2014

I would prefer consistency between our Ruby guides and CoffeeScript guides. So whatever guideline we currently have there (single quotes are still preferred, I believe), then we should make the same guideline here.

@dgalarza
Copy link

I agree with @mcmire. I think we should be consistent with our style between CoffeeScript and Ruby with things like quotation marks.

@sgrif
Copy link
Contributor

sgrif commented Jun 13, 2014

Keep in mind, it looks like the ruby guide is about to switch to double quotes. #174

@kaishin
Copy link

kaishin commented Jun 14, 2014

I can live with double quotes in Coffeescript.

@jferris
Copy link
Contributor

jferris commented Jun 16, 2014

Unless there's a strong reason to diverge, I'd prefer to be consistent with what we're doing in Ruby.

@mxie
Copy link

mxie commented Jun 16, 2014

Yes, double quotes for all!

@sgrif
Copy link
Contributor

sgrif commented Jun 16, 2014

Should we promote this to a generic "if your language supports both single
and double quotes..."?
On Jun 16, 2014 8:33 AM, "Melissa Xie" [email protected] wrote:

Yes, double quotes for all!


Reply to this email directly or view it on GitHub
#187 (comment).

@jferris
Copy link
Contributor

jferris commented Jun 16, 2014

Should we promote this to a generic "if your language supports both single
and double quotes..."?

Would you think to check both "Coffeescript" and "Generic" sections? I'm not sure that I would.

@mcmire
Copy link

mcmire commented Jun 16, 2014

I think it's fine to keep guidelines separate for each language. There are people who know Ruby but not CoffeeScript and vice versa (not among us, likely, but elsewhere).

@jasonramirez
Copy link
Contributor Author

+1 for keeping this specific to each language

@jferris
Copy link
Contributor

jferris commented Jun 23, 2014

Any objections to merging this as-is?

@sgrif
Copy link
Contributor

sgrif commented Jun 23, 2014

👍

@mcmire
Copy link

mcmire commented Jun 23, 2014

I'm in favor.

@jasonramirez
Copy link
Contributor Author

Merging

* Use `@` instead of `this` for scoping consistency
* Prefer double quotes for consistency
@jasonramirez jasonramirez merged commit ef7b36a into master Jun 27, 2014
@tysongach tysongach deleted the jr-coffee-style-update branch March 14, 2015 15:07
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.

10 participants