2
\$\begingroup\$

A coworker and I want to get feedback on our respective implementations of a JavaScript function. Both seem fast, but we're wondering which is better stylistically.

Both functions handle what icons appear in a control panel when navigating between sections inside of an administrator dashboard.

The first function

function OLD_iconControls(selectedMenu){
    // Cache the selectors
    var icons        = $('#sdvControlPanel .iconControls');
    var menuItem     = $('.menuitem');
    var iconAdd      = $('#iconAdd');
    var iconEdit     = $('#iconEdit');
    var iconSave     = $('#iconSave');
    var iconUndo     = $('#iconUndo');
    var iconProcess  = $('#iconProcess');
    var iconFirst    = $('#iconFirst');
    var iconPrevious = $('#iconPrevious');
    var iconNext     = $('#iconNext');
    var iconLast     = $('#iconLast');
    var controlPanel = $('#controlPanelAlign');
    var sectionValue = selectedMenu.attr('val');

    // Store the selectors for each menu item in variables
    var help         = selectedMenu.attr('0');
    var student      = selectedMenu.attr('1');
    var residence    = selectedMenu.attr('2');
    var contact      = selectedMenu.attr('3');
    var registration = selectedMenu.attr('9');
    var enroll       = selectedMenu.attr('4');
    var minireport   = selectedMenu.attr('5');

    // Hide all the icons in the left control panel
    icons.css('display', 'none');

    // First we display the control panel divs all at once
    controlPanel.css('display', 'block');

    // Show Edit, Save, and Undo icons. These are shown in every section
    iconSave.css('display', 'inline-block');
    iconEdit.css('display', 'inline-block');
    iconUndo.css('display', 'inline-block');

    // Then we display the icons within the alignment div based on the section selected.
    if      ( help ){
        controlPanel.css('display', 'none');
    }
    else if ( student ){
    }
    else if ( residence ){
    }
    else if ( contact ){
        iconAdd.css('display', 'inline-block');
    }
    else if ( registration ){
    }
    else if ( enroll ){
        iconProcess.css('display', 'inline-block');
    }
    else if ( minireport ){
    }
}

The second function

function iconControls(menuitemclicked){
    // Used to show the proper control icons in the floating menu div(s).

    // Store the value of the selected control.
    var selected_val = $(menuitemclicked).attr('val');

    // Using these variable names as a sort of named-constant. - Readability!
    var help         = '0';
    var student      = '1';
    var residence    = '2';
    var contact      = '3';
    var registration = '9';
    var enroll       = '4';
    var minireport   = '5';

    // Cache the selector as I did in he OLD function. Better coding. Handles the control div positioning as you switch between sections.
    var bottomReset = $('#controlPanelAlign .controlPanels').css({bottom: '0px', position: 'absolute'});

    // Hide all the icons in the left control panel
    $('#sdvControlPanel .iconControls').css('display', 'none');

    // First we display the control panel divs all at once
    $('#controlPanelAlign').css('display', 'block');

    // Show Edit, Save, and Undo icons. These are shown in every section
    $('#iconSave').css('display', 'inline-block');
    $('#iconEdit').css('display', 'inline-block');
    $('#iconUndo').css('display', 'inline-block');

    $('#controlPanelAlign').css('position', 'initial');

    // Then we display the icons within the alignment div based on the section selected.
    if      ( selected_val == help )        {
        console.log("help");
        $('#controlPanelAlign').css('display', 'none');
    }
    else if ( selected_val == student )     {
        console.log("student");
        bottomReset;
    }
    else if ( selected_val == residence )   {
        console.log("residence");
        bottomReset;
    }
    else if ( selected_val == contact )     {
        console.log("contact");
        bottomReset;
    $('#iconAdd').css('display', 'inline-block');
    }
    else if ( selected_val == registration ){
        console.log("registration");
        bottomReset;
    }
    else if ( selected_val == enroll )      {
        console.log("enroll");
        $('#iconProcess').css('display', 'inline-block');
        $('#controlPanelAlign .controlPanels').css({bottom: 'auto', position: 'absolute'});
    }
    else if ( selected_val == minireport )  {
        console.log("minireport");
        bottomReset;
    }
}
\$\endgroup\$
  • \$\begingroup\$ This question has been cross-posted from Stack Overflow stackoverflow.com/questions/31566438/… (where it got closed as "too broad") \$\endgroup\$ – Simon Forsberg Jul 22 '15 at 16:59
  • \$\begingroup\$ The second option seems quite a bit cleaner to me. I'd suggest changing the long else if to a switch statement and move the assignment of all the constants outside the function so it isn't executed everytime you run. Also, what is bottomReset; supposed to be doing? Is that supposed to be a function call? \$\endgroup\$ – jfriend00 Jul 22 '15 at 17:01
  • 3
    \$\begingroup\$ Please add the HTML that this code is operating against. \$\endgroup\$ – 200_success Jul 22 '15 at 17:03
  • \$\begingroup\$ jfriend - bottomReset is a variable just under the minireport variable. \$\endgroup\$ – Tom Danford Jul 22 '15 at 17:09
  • \$\begingroup\$ Because no one else mentioned it, I would recommend moving away from directly modifying the css via jQuery. For instance, $(el).css('display','none'); is pretty much the same as as using $(el).hide();. Better yet, create a .hidden class like .hidden { display:none;} and just add it to the elements you want to hide. Remove the class when you want to display it. \$\endgroup\$ – Gary Storey Jul 28 '15 at 16:58
1
\$\begingroup\$

The first one

if      ( help ){
    controlPanel.css('display', 'none');
}
else if ( student ){
}
else if ( residence ){
}
else if ( contact ){
    iconAdd.css('display', 'inline-block');
}
else if ( registration ){
}
else if ( enroll ){
    iconProcess.css('display', 'inline-block');
}
else if ( minireport ){
}

What is the purpose of keeping all the values distinct from one another if they're mutually exclusive? Having them separated rather than using a single variable (in the second one's case, selected_val) can open the door to issues involving mistaken intent when more than one condition is true. Though this likely won't be a problem with a menu where a user can only choose one, it is something that could be problematic elsewhere.

Unless there's a specific purpose to the order this appears in, the if-statements that have nothing in them (so student, residence, and minireport) can be removed. Only one of the conditions can hold true, so having them as distinct if-statements is unnecessary.

Also, why cache selectors like iconNext or iconPrevious? They aren't used anywhere else in the function, so unless their blocks of memory are used elsewhere that isn't shown here, it seems unnecessary. Whether or not caching them brings any benefit is really a matter of how this particular function is used in the code not shown here--if they get taken out of the cache after each call, trying to cache things really brings no benefit.

The second one

Unless the console.logs are necessary, they can be removed and several of the if-statements can be combined to avoid repetition.

\$\endgroup\$
  • \$\begingroup\$ The menu system is only looking for one value of the clicked .menuitem at a time. Each HTML .menuitem element has a "val=#" set as a property and the function looks for those values and does stuff based on that. You're right, there's no priority on the order they appear in. I simply wrote the empty if statements in case we wanted to add functionality later we had the structure built. But I see that can be done regardless without adding extra code. Same goes with why we cached the unused icon selectors like iconPrevious. So I tidied it up a bit: jsfiddle.net/9ak44ohc/1 \$\endgroup\$ – Tom Danford Jul 22 '15 at 17:38
1
\$\begingroup\$

This changes applies for both files but I changed just the first one.

You should pay attention to comments.

var config = {
    iconSelectors: {
        icons: $('#sdvControlPanel .iconControls'),
        menuItem: $('.menuitem'),
        addIcon: $('#iconAdd'),
        editIcon: $('#iconEdit'),
        saveIcon: $('#iconSave'),
        undoIcon: $('#iconUndo'),
        processIcon: $('#iconProcess'),
        firstIcon: $('#iconFirst'),
        previousIcon: $('#iconPrevious'),
        nextIcon: $('#iconNext'),
        lastIcon: $('#iconLast'),
        controlPanelAlign: $('#controlPanelAlign')
    },
    // Don't know why you got this attributes on the selectedMenu but ...
    setAttributes: function(selectedMenu) {
        return {
            help: selectedMenu.attr('0'),
            student: selectedMenu.attr('1'),
            residence: selectedMenu.attr('2'),
            contact: selectedMenu.attr('3'),
            registration: selectedMenu.attr('9'),
            enroll: selectedMenu.attr('4'),
            minireport: selectedMenu.attr('5')
        }
    }
};

function OLD_iconControls(selectedMenu) {
    // This is NOT used !
    var sectionValue = selectedMenu.attr('val');
    var attributes = config.setAttributes();
    // Hide all the icons in the left control panel
    config.iconSelectors.icons.css('display', 'none');

    // First we display the control panel divs all at once
    config.iconSelectors.controlPanelAlign.css('display', 'block');

    // Show Edit, Save, and Undo icons. These are shown in every section
    config.iconSelectors.saveIcon.css('display', 'inline-block');
    config.iconSelectors.editIcon.css('display', 'inline-block');
    config.iconSelectors.undoIcon.css('display', 'inline-block');

    // Then we display the icons within the alignment div based on the section selected.
    if (attributes.help) {
        config.iconSelectors.controlPanelAlign.css('display', 'none');
    } else if (attributes.student) {
        // Do something!
    } else if (attributes.residence) {
        // Do something!
    } else if (attributes.contact) {
        iconAdd.css('display', 'inline-block');
    } else if (attributes.registration) {
        // Do something!
    } else if (attributes.enroll) {
        config.iconSelectors.processIcon.css('display', 'inline-block');
    } else if (attributes.minireport) {
        // Do something!
    }
}
  • The code is more readable
  • You have an object (maybe on the top of the file) where other programmer will look for some jQuery object and add a new one if you have more icons in the future
  • The function code is more concise and you'll understand what it does just reading it
\$\endgroup\$
  • \$\begingroup\$ You forgot to mention that attributed 0...9 are invalid. It must be data-0, data-1, ... data-9. \$\endgroup\$ – Ismael Miguel Jul 25 '15 at 15:48

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.