1
\$\begingroup\$

I have some of my code that I have tried to improve on to make it cleaner and efficient and I would like some feedback to see if I am going in the right direction with what I am trying to do.

I have a page with a menu, and when you click on menu item it displays the appropriate fieldset.

HTML:

<!-- dashboard_container -->
<div id="dashboard_container" class="">

    <!-- Dashboard Menu -->
    <div id="dashboard_menu" class="">
            <!-- Menu list-->
            <ul id='side_bar_menu'>
                <li id='list_account_details' data-fieldset-id='#fs_account_details'>My Account</li>
                <li id='list_order_history' data-fieldset-id='#fs_order_history'>Order History</li>
                <li id='list_notifications' data-fieldset-id='#fs_notifications'>Notifications</li>
            </ul>   
    </div>

    <!-- Dashboard Content -->
    <div id="dashboard_content" class="">

        <!-- Account details fieldsets -->
        <fieldset id="fs_account_details"> 
            <!-- content here -->
        </fieldset>

        <!-- Order History fieldsets -->
        <fieldset id="fs_order_history"> 
            <!-- content here -->
        </fieldset>

        <!-- Notifications fieldsets -->
        <fieldset id="fs_notifications"> 
            <!-- content here -->
        </fieldset>

    </div>
</div>

My original jQuery to display each fieldset was an individual block of code for each button like this:

// Show account details
$('#list_account_details').click( function(){
    $('fieldset').hide( function(){
        $('#fs_account_details').show();    
    });
});

// Show order history
$('#list_order_history').click( function(){
    $('fieldset').hide( function(){
        $('#fs_order_history').show();  
    });
});

// Show notifications
$('#list_notifications').click( function(){
    $('fieldset').hide( function(){
        $('#fs_notifications').show();  
    });
});

And my improvement was to only have one block of code to display each fieldset like this:

// Show appropriate fieldset
$('#side_bar_menu li').click( function(){

    // Get button id
    var button_id = $(this).attr('data-fieldset-id');

    // Hide any opened fieldsets
    $('fieldset').hide( function(){
        // Show appropriate fieldset
        $(button_id).show();            
    });
});

Should I be thinking to automatically write all my code like this, or am I possibly reducing individual functionality of each menu list button by incorporating it all into one block of code like that?

Is my new code even considered an improvement at all?

\$\endgroup\$
1
\$\begingroup\$

I think, your improved code looks okay temporarily until you try to add new functionalities.

A function should do only one thing.

Trying to add a custom functionality to your improved code would result in clutter and more if else statements.

\$\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.