CODEWITHSUNDEEP
javascriptjqueryoop
nickcoxdotme
nickcoxdotme

Reputation: 6697

A more elegant/DRY/maintainable JS solution here?

I initially began my task by trying to use JS best practices when writing this code, but I was struggling, so I just wrote it as a big sloppy mess to get it to work, and then tried to clean it up after the fact. (You may argue that that wasn't a good idea, but it was the best I could do with where I am.)

My boss suggested I try to use custom triggers and events, but that didn't really seem the best route. I also read this article by Rebecca Murphy on OOJS that seemed like it might have an answer, but I just couldn't get there.

Keep in mind that, while I'm definitely specifically looking for a more elegant (and less nested-if-statement-dependent) way to write this and definitely appreciate any help on that matter, I'm also looking for patterns that could help me address my larger stylistic shortcomings here and become a better programmer. For example, I know that this is the clunkiest and slowest way to traverse the DOM.

One important note: I can't control much of the markup (aside from adding classes as JS/CSS hooks) in this situation, because it's being generated by the SimpleForm Ruby gem (I'm unable to link to it because of my low score). And the CSS is irrelevant. So it's really just the JS.

Context

Certain things are being shown/hidden based on select or radio button choices.

EDIT: Sorry, my HTML was getting weird here, so I just made a jsFiddle.

EDIT 2: We're using jQuery 1.6.4, so if you're wondering why I used .delegate instead of .on, that's why.

Upvotes: 0

Views: 103

Answers (4)

Bergi
Bergi

Reputation: 664307

A few points:

  • The no-information option should be selected by default, instead of an empty one. You also have no handler for selecting that one - I suggest removing it.
  • You should use ids instead of classes where appropriate (In here: the living, deseased, cancer-info and other-illnesses divs, the deceased-select etc.
  • You should really not rely on the indexes of the selected options in the dom - they may change. Instead, use the selected value to determine the actions: $(".deceased-select").val()
  • The selection of the .next() control-group looks unreliable. Better select explicitly what you want.
  • There is no need to use delegated events anywhere. Replace the live with on, on the delegate-thing with a simple $('.had-cancer-radio').on(…
  • You can reduce you if-else statements a lot:

var value = $(".deceased-select").val(),
    other = $("#other-illnesses, #cancer-info"),
    deceased = container.siblings('.deceased'),
    living = container.siblings('.living');
if (value == "no-information") {
    other.hide();
} else {
    other.slideDown('fast');
    if (value == "living") {
        living.slideDown('fast');
        deceased.hide();
    } else /* if (value == "deceased") */ {
        deceased.slideDown('fast');
        living.hide();
    }
}

Upvotes: 0

Alnitak
Alnitak

Reputation: 339786

There's a few obvious things:

  1. don't evaluate $(this) repeatedly - do it once, and cache the result
  2. likewise for selected.index()
  3. use a simple wrapper for .slideUp and .slideDown('fast') so that you don't repeat those over and over, and can change the animation in just one place if required.

The final block can be written thus:

var $next = $(this).closest('.control-group').next();
if ($(this).val() === 'true') {
    $next.slideUp();
} else {
    $next.slideDown();
}

In fact, the whole if block can be written like this, although some may find this a little too terse:

$next[$(this).val() === 'true') ? 'slideDown' : 'slideUp']();

Upvotes: 0

Kostia
Kostia

Reputation: 6304

You could also keep things DRY by removing the duplicated code to show/hide items. Here is a very simplistic approach that improves things:

function changeElements (show_elements, hide_elements) {
    if (show_elements === null) {
        // code to hide all
    }

    container.siblings(show_elements).slideDown();
    container.siblings(hide_elements).slideUp();
}

and then to use the function, simply give it the selectors to show, and to hide respectively:

changeElements('.deceased', '.living, .other-illnesses, .cancer-info');

Upvotes: 1

machineghost
machineghost

Reputation: 35760

Two little things (besides the two Alnitak mentioned):

1) Don't use live (see http://bitovi.com/blog/2011/04/why-you-should-never-use-jquery-live.html, or any of the SO posts on the topic)

2) $(document).ready(function(){ can just be $(function(){

That aside, there's nothing fundamentally wrong with your code, and honestly for such a small chunk I think you're probably over-thinking things. If you were using say Backbone views for each of those elements, then you might want to switch to an event-driven approach, but that would be overkill for something as small/simple as what you have.

Upvotes: 0

Related Questions