Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is based (somewhat loosely) on the code written in this railscast. I'm planning to re-use it in different places throughout the site, so I made it a bit more generic rather than based on IDs. However, I'm certain there's probably room for improvement. Written in CoffeeScript.

jQuery ->
  $('.org_type_choice').each ->
    cat_obj = $(this).find('.org_category')
    type_obj = $(this).find('.org_type')
    categories = cat_obj.html()

    org_type = type_obj.find(':selected').text()
    opts = $(categories).filter("optgroup[label='#{org_type}']").html()
    cat_obj.html(opts)

    type_obj.change ->
      org_type = $(this).find(':selected').text()
      opts = $(categories).filter("optgroup[label='#{org_type}']").html()
      cat_obj.html(opts)
share|improve this question

1 Answer 1

up vote 4 down vote accepted

I see a couple of small ways to improve this.

  1. DRY (Don't Repeat Yourself) the code. The code that runs on load is identical to the change event handler. So make it a function you can call from both places.

  2. Decouple it completely from the markup by dropping the "org-*" classes. It may not be "orgs" with "categories" and "types" that you're trying to select, but, say, "cars" with "makes" and "models". The basic idea is simply that there's a primary select element, and it scopes (filters) a secondary select element.

For the second point, I'd use a custom data-* attribute on the primary select. You might call it data-secondary, and let its value be the ID of the secondary select element. E.g.

<select data-secondary="categories">...</select>
<select id="categories">...</select>

Now you can find all select elements that have an associated secondary select element, by saying $("select[data-secondary]"). From there, you can find that secondary, and use the current logic. The markup declares the relationship and behavior, while the code can stay completely generic.

I end up with code like this:

jQuery ->
  $("select[data-secondary]").each ->
    primary   = $ this
    secondary = $ "##{primary.data 'secondary'}" # find the secondary
    items     = secondary.clone()                # clone the secondary into memory

    updateSecondarySelect = ->
      scope = primary.find(":selected").text()
      secondary.html items.find("optgroup[label='#{scope}']").html()

    primary.on "change", updateSecondarySelect # set up the event listener
    updateSecondarySelect()                    # do the initial filtering

(I've changed to CamelCase, but that's not important; use whatever style you prefer)

Some extra error checking would be good (e.g. what happens if the secondary isn't found? If there's no optgroup matching the selected option in the primary, should the secondary maybe just be hidden? etc).

Here's a demo

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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