Search code examples
javascriptjqueryperformancedry

Suggestions to make this jQuery function more DRY / more efficient

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');
    }

});
Solution

  • Several points to make it more DRY:

    1. Use only one var keyword, and separate the items with commas. Ex. var asdf = 1, sdfg = '', dfgh = true;
    2. Use multiple selectors so you apply the .addClass action only once. See https://api.jquery.com/multiple-selector/
    3. 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()
    4. 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.
    5. 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.