CODEWITHSUNDEEP
javascriptjquerycoffeescript
Steve Ross
Steve Ross

Reputation: 4144

Coffeescript/jQuery Pattern: Caching Data Across Events

I have a list of items that are abstracts, and can be made to expand via Ajax on click. I'v written the following code in Coffeescript:

current_open_row = null

$('li.faq-item').live 'click', (event) ->
  $.post("/faqs/update_rows", {old_row_id: current_open_row, new_row_id: $(this).attr('id')}, (data) ->
    replace_items data
  , 'json')
  current_open_row = $(this).attr('id')

This doesn't read like smooth Coffeescript and I find myself thinking, "what could I have done better," but in particular, instantiating the current_open_row variable outside the scope of the click handler feels strange. Not doing this, of course, causes a new instantiation upon entry to the handler, which is always undefined.

Other than refactoring $(this).attr('id') into a variable, is there anything that jumps out as ugly, suboptimal, unreadable, etc., or is this pretty much the way of it?

Thanks!

Upvotes: 2

Views: 3753

Answers (1)

Trevor Burnham
Trevor Burnham

Reputation: 77416

Well, first off, I think you'll find yourself switching to camelCase eventually... I know that many people strongly prefer the readability_of_underscores, but every library you interact with (including jQuery) uses camelCase. Just something to keep in mind.

Setting that aside, the issue of having to scope variables with = null is a good one. I've tried to persuade Jeremy that there should be a nicer scoping syntax, but he's firmly against it. So I'd suggest moving the variable to an object property. Fortunately, this is jQuery, so there are plenty of places to stick data. How about using the list's .data method? This has an additional advantage: If you want to do this on multiple lists in the future (with one current_open_row in each list), you won't have to change your code one bit. Just add more lists with .faq-item children to the markup.

One more minor point: You give the post call a callback

(data) -> replace_items data

If that's all you're doing, why not just pass replace_items directly? :)

I'd also put each argument to the post function on its own line for readability. Key-value pairs are automatically combined into a single object, even without curly braces. Here's how it all looks:

$('li.faq-item').live 'click', (event) ->
  $row = $(this)
  $list = $row.parent()
  row_id = $row.attr 'id'

  $.post(
    "/faqs/update_rows",
    old_row_id: $list.data('current_open_row'),
    new_row_id: row_id,
    replace_items,
    'json'
  )

  $list.data 'current_open_row', row_id

Upvotes: 2

Related Questions