1
\$\begingroup\$

I just wrote this, and cleaned it up as much as I could. A couple things worry me:

  • I change the scope of $(this) twice (and apparently don't know how to use =>)
  • The big if if/else statement. Is there a better way to write that?

Also, it's an IIFE, and the big identing makes the )() at the end look out of place.

  emptyTagFieldsOnDeletion = (->
    $('form.simple_form').submit ->
      $form = $(this)
      $form.find('.fake-input').each ->
        $fakeInput = $(this)
        $ulId = $fakeInput.closest('ul').attr('id')
        if $fakeInput.children('li').length is 1
          $fakeInput.children().remove()
          if $ulId is 'primary_diagnosis_select'
            $fakeInput.append emptyListArrayHTML('primary_icd9_codes')
          else if $ulId is 'secondary_diagnosis_select'
            $fakeInput.append emptyListArrayHTML('secondary_icd9_codes')
          else
            $fakeInput.append emptyListArrayHTML('med_names')
  )()

For reference, the emptyListArrayHTML method looks like this:

emptyListArrayHTML = (list_type)->
  "<input type='text' name='#{nameWithListType(list_type)}' value=''>"
\$\endgroup\$
2
\$\begingroup\$

You mention the changes in scope as though they're a problem. But you actually want them here, since that's how jQuery operates, and using => would be counterproductive. Anyway here are some notes

  1. CoffeeScript has a do keyword for IIFEs. So do -> ... is equivalent to (-> ...)() without the need for all those parentheses

  2. It seems your IIFE isn't returning something you'll need later; it's just encapsulating some code. In that case, don't bother giving it a name. Just invoke it.

  3. You don't need to cache $form - you only use it once

  4. Don't call the UL id $ulId - that is, don't use a $ in the name. It's not a jQuery object, it's just a string

  5. You can use a switch statement instead of the if... else, which helps a bit (but read on for a better solution)

Here's the code with these tweaks:

do ->
  $('form.simple_form').submit ->
    $(this).find('.fake-input').each ->   # don't bother caching $form
      $fakeInput = $ this
      return unless $fakeInput.children().length is 1 # return early instead of a big if-block

      $fakeInput.empty()

      switch $fakeInput.closest('ul').attr('id')
        when 'primary_diagnosis_select'
          $fakeInput.append emptyListArrayHTML('primary_icd9_codes')
        when 'secondary_diagnosis_select'
          $fakeInput.append emptyListArrayHTML('secondary_icd9_codes')
        else
          $fakeInput.append emptyListArrayHTML('med_names')

However, I'd make the relationships explicit in the markup. For instance, give the UL element a data-codes attribute or something:

<ul id="primary_diagnosis_select" data-codes="primary_icd9_codes">
...
<ul id="secondary_diagnosis_select" data-codes="secondary_icd9_codes">
...
<ul id="something_else"> <!-- no data-codes for this one; defaults to "med_names" in the CoffeeScript code -->

Then you can shorten the code like so:

do ->
  $('form.simple_form').submit ->
    $(this).find('.fake-input').each ->
      $fakeInput = $ this
      return unless $fakeInput.children().length is 1
      $fakeInput.empty()
      codes = $fakeInput.closest('ul').data("codes") or "med_names"
      $fakeInput.append emptyListArrayHTML(codes)

Now the CoffeeScript handles only logic while the data and relationships are declared in the markup. You can add the single line from emptyListArrayHTML back into the function with complicating things too much, though.

p.s. CoffeeScript also has a triple-quote Python-like heredoc syntax, if you want to use double quotes in a string without escaping them. So your emptyListArrayHTML function could be written as

emptyListArrayHTML = (list_type) ->
  """<input type="text" name="#{nameWithListType list_type}" value="">"""

It looks a little strange at first (and the syntax highlighting is messed up here), but I like to use it, because I prefer to always use double quotes for HTML attributes (just personal preference).

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Yep. All great suggestions. Just what I was looking for! Thanks. \$\endgroup\$ – nickcoxdotme Jun 18 '13 at 22:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.