Suggestions to make this jQuery function more DRY / more efficient

Question

Following previous post the this code works and does the job but I am conscious this is about as DRY as the Pacific on a wet day.

I's be grateful for any suggestions that will make it more efficient.

$( "#cvl_mb_services .content-switch" ).each(function(index, el) {
    var parent        = $(el).parent().parent().attr("id");
    var inputValue    = $('#' + parent + ' input[type=radio]:checked').val();
    var targetBox   = '#' + parent + ' .cvl-' + inputValue + '-fields';

    $(targetBox).removeClass('cvl-hide');
});


$('#cvl_mb_services .content-switch').on('click', 'input[type="radio"]', function(){

    var parent      = $(this).parent().parent().parent().parent().parent().parent().attr("id");
    var inputValue  = $(this).closest('input[type="radio"]').attr("value");
    var targetBox   = '#' + parent + ' .cvl-' + inputValue + '-fields';

    if (inputValue == 'content') {
        $('#' + parent + ' .cvl-content-fields').removeClass('cvl-hide');
        $('#' + parent + ' .cvl-header-fields').addClass('cvl-hide');
        $('#' + parent + ' .cvl-footer-fields').addClass('cvl-hide');
    } else if (inputValue == 'header') {
        $('#' + parent + ' .cvl-content-fields').addClass('cvl-hide');
        $('#' + parent + ' .cvl-header-fields').removeClass('cvl-hide');
        $('#' + parent + ' .cvl-footer-fields').addClass('cvl-hide');
    } else if (inputValue == 'footer') {
        $('#' + parent + ' .cvl-content-fields').addClass('cvl-hide');
        $('#' + parent + ' .cvl-header-fields').addClass('cvl-hide');
        $('#' + parent + ' .cvl-footer-fields').removeClass('cvl-hide');
    }

});

I’m voting to close this question because it belongs on https://codereview.stackexchange.com/ — freedomn-m, Jul 22 '20 at 13:33
Couple of possible improvements: DOM navigation will always be the slowest, so cache any DOM lookups, eg `var parent = $("#" + $(this).parent()....attr("id")); parent.find(".cvl-content-fields, .cvl-header-fields).addClass`. Use `.closest()` instead of `.parent().parent().parent()` so it's not so brittle. Combine class changes (as in I've shown earlier). But these are micro improvements that won't really make much difference on a `click` handler - you'll notice if your running code like this 1000s of times a second. — freedomn-m, Jul 22 '20 at 13:36

Shawn · Accepted Answer · 2020-07-22T17:10:25.333

Several points to make it more DRY:

Use only one var keyword, and separate the items with commas. Ex. var asdf = 1, sdfg = '', dfgh = true;
Use multiple selectors so you apply the .addClass action only once. See https://api.jquery.com/multiple-selector/
Find a way to get rid of this duplication, such as perhaps adding/using a class to select the right ancestor: .parent().parent().parent().parent().parent().parent()
Don't duplicate strings like 'cvl-hide' Instead make a variable. Many JavaScript minifiers won't touch strings, so you'll end up with this duplication making your overall file larger than it needs to be.
Make variables for duplicate selectors so jQuery doesn't have to do the same lookup twice. For stuff like $('#cvl_mb_services .content-switch') and even for stuff like $(this) which is duplicated.

One alternative solution (that isn't a direct answer to the question but may solve the use case) is to change the css so that when you apply a class to the parent node, the _css_ causes the objects to hide and show. so then all you have to do is add a css class to _one_ node. That may or may not make more sense. — Garrett Motzner, Jul 22 '20 at 16:59

Suggestions to make this jQuery function more DRY / more efficient

1 Answers1

Linked