1
\$\begingroup\$

I will admit that this code looks sloppy and repetitive. Which is why I'm asking how I can improve the efficiency and readability of this code.

// Sidebar tab highlight when clicked
$("#sidebar-tabs-container table td").click(function() {
  $("#sidebar-tabs-container table td").removeClass("sidebar-tab-selected");
  $(this).addClass("sidebar-tab-selected");
});



// poll and faq
// when faq question bar is clicked, slide down answer
$(document).on("click", ".question-container", function() {
$(".answer-container").stop().slideUp("fast");
$(this).parent().find(".answer-container").stop().slideToggle("fast");
});




// news and social feed





// activity
// activity new media content 
//media buttons click 
$(document).on("click", "#activity-media-icons-container span", function() {
    $("#activity-media-icons-container span").removeClass("activity-new-media-icon-  clicked");
$(this).addClass("activity-new-media-icon-clicked");
});

// show new media category and hide others depending on button clicked
$(document).on("click", "#new-media-images-button", function() {
   $("#activity-media-video-container, #activity-media-audio-container").slideUp(    function() {
   $("#activity-media-images-container").slideDown();
   });
});

$(document).on("click", "#new-media-videos-button", function() {
$("#activity-media-audio-container, #activity-media-images-container").slideUp( function() {
    $("#activity-media-video-container").slideDown();
});
});

$(document).on("click", "#new-media-audio-button", function() {
$("#activity-media-images-container, #activity-media-video-container").slideUp( function() {
    $("#activity-media-audio-container").slideDown();
});
});

/* show image link icon when hovered over image */
$(document).ajaxComplete(function() {
$(".media-image-buttons-container").hide();
});
// fade in image buttons on hover
$(document).on("mouseenter", "#media-content-container .media-image-container", function() {
$(this).children(".media-image-buttons-container").stop().fadeIn(200);
});
$(document).on("mouseleave", "#media-content-container .media-image-container", function() {
$(this).children(".media-image-buttons-container").stop().fadeOut(200);
});
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

This code can use quite a bit of work.

  • Run this code through a beautifier, ideally prior to posting it to codereview ;)
  • You are repeating yourself a lot, example :
// Sidebar tab highlight when clicked
$("#sidebar-tabs-container table td").click(function() {
  $("#sidebar-tabs-container table td").removeClass("sidebar-tab-selected");
  $(this).addClass("sidebar-tab-selected");
});

Unless you have a dynamic number of sidebar tabs ( seems unlikely ), you can do the following.

var $sidebarTabs  = $('#sidebar-tabs-container table td');
var selectedClass = 'sidebar-tab-selected';

$sidebarTabs.click(function() {
  $sidebarTabs.removeClass( selectedClass );
  $(this).addClass( selectedClass );
});

The interesting part is that you do this type of code twice, so you could also write a helper.

function wireClassToggler( elements, className )
{
  elements.click(function()
  {
    elements.removeClass( className );
    $(this).addClass( className );
  });
}

wireClassToggler( $('#sidebar-tabs-container table td'), 'sidebar-tab-selected' );
wireClassToggler( $('#activity-media-icons-container span'), 'activity-new-media-icon-clicked' );

You also seem to use copy pasted code for the slideUp and slideDown functions, you should generalize that as well.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.