Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a link (avatar)#qam-account-toggle and div.qam-account-items, the div containing user account links. I am toggling div on avatar click as well as click anywhere but div.qam-account-items

Here is my code. How can I optimize this better?



share|improve this question

Not much to say here, it looks pretty good. I've moved to only using .on for attaching events as I think it encourages better practices. Remember it's better to save selectors as variables if you are going to reuse them.

Instead of catching the click on .qam-account-items you can just fire the event on everything but those classes;

$(document).on('click', '*:not(.qam-account-items)', function() {
share|improve this answer
Thanks for the answer but this code is not working. stopped show hide.. any idea why is that? – pixelngrain Mar 31 '14 at 16:08
I see this code is only for ignoring qam-account-items but it is still hiding it when I click on input box within the div – pixelngrain Mar 31 '14 at 16:18
@pixelngrain Yeah you're right... I'll mull it over. – Jivings Mar 31 '14 at 18:44
Any idea how to fix it? – pixelngrain Apr 3 '14 at 7:19

I'd suggest wrapping the code in a function, so it's all in one place and can share variables. Then there's no need to find and re-find the same elements in each event handler.

Also, you're doing some extra filtering by class name and such which is unnecessary. E.g. .hide() will hide whatever's visible, so you don't need to first use :visible to find the visible elements.

function enableAccountItemsToggling() {
  // find these once
  var accountToggle = $('#qam-account-toggle'),
      accountItems  = $('.qam-account-items');

  // toggle items
  accountToggle.on("click", function (event) {

    // a single click is enough; no need for a persistent event handler
    // so we use .one() instead of .on()
    $(document).one("click", function () {

  accountItems.on("click", function (event) {
share|improve this answer

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

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