3
\$\begingroup\$

What is the best way to refactor the following script?

<script type="text/javascript">

    $(document).ready(function() {
        $(".rolesList").hide(); 
    });

        $("#legendFunction").click(function() {
        $("#divChkUserRoles").toggle('slow');            
            var text = $("#lblExpandFunction").text() == '+' ? '-' : '+';
            $("#lblExpandFunction").text(text);
        })

        $("#legendMISheet").click(function() {
        $("#divChkUserRolesMISheet").toggle('slow');
            var text = $("#lblExpandMISheet").text() == '+' ? '-' : '+';
            $("#lblExpandMISheet").text(text);
        })

</script>

The above script will be applied to the following HTML and ASP.NET code:

            <fieldset>
                <legend><span id="legendFunction" style="cursor: pointer" title="Click here to toggle show or collapse Function.">
                    <label id="lblExpandFunction" style="padding: 5px;">+</label>
                    KengLink Function 
                </span></legend>
                <div id="divChkUserRoles"  class="rolesList">      
                    <asp:CheckBoxList ID="UserRoles" runat="server" />                  
                </div>
            </fieldset>
            <fieldset>
                <legend><span id="legendMISheet" style="cursor: pointer" title="Click here to toggle show or collapse MI Sheet.">
                    <label id="lblExpandMISheet" style="padding: 5px;">+</label>
                MI Sheet
                </span></legend>
                <div id="divChkUserRolesMISheet" class="rolesList">  
                    <asp:CheckBoxList ID="UserRolesMISheet" runat="server" />
                </div>
            </fieldset>
\$\endgroup\$
3
\$\begingroup\$

Don't use ids at all. (You almost never should be using them. Ids get to be really problematic once you have composite views and/or multiple people on a project)

Place a few appropriate classes in your html. And use the jquery composite and relative references to do it all at once.

$('.expander').click(function() {
  $(this).closest('fieldset').find('.expandable').toggle('show');
  var l = $(this).find('label');
  l.text(l.text() === '+' ? '-' : '+');
});

where your clicker span gets a class of expander and your rolesList gets the class of expandable. You could just use rolesList directly, but I find expandable to be somewhat more descriptive.

\$\endgroup\$
  • \$\begingroup\$ Skillful solution, anyway, Why class not be a problematic as id too. \$\endgroup\$ – Sarawut Positwinyu Mar 27 '12 at 15:28
  • 1
    \$\begingroup\$ @SarawutPositwinyu many libraries assume that only one instance of an id exists on a page. Once you start composing views or having multiple people its too easy to put several of the same ids on one page and get really weird errors. With classes you have to assume that there is more than one and you have to take that into account. Also, the fact that you can have multiple classes on an instance usually guides people to give things more meaningful names. \$\endgroup\$ – George Mauer Mar 27 '12 at 19:49
  • \$\begingroup\$ Thx, Oh class name should be '.expandable' instead of 'expandable' (in the code). \$\endgroup\$ – Sarawut Positwinyu Mar 28 '12 at 3:22
0
\$\begingroup\$

In addition to George solution, you can also use on()/off() event API rather then using click(), on/off much faster then other jQuery event handlers.

And rather then calling anonymous function inside click(), use named function to debug it easily later on, it's always better to write the JavaScript code in chunks/modules for the sake of modularity.

\$\endgroup\$

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.