Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here's my piece of code

$j('#hideShowBtn').toggle(function() {
     $j('.playlist-bar-tray').slideUp();
     $j(this).attr("title","Show Playlist");         
     $j(this).children("div").html('Show Playlist');
     $j(this).children("span").html('▼');
 }, function() {
     $j('.playlist-bar-tray').slideDown();
     $j(this).attr("title","Hide Playlist");
     $j(this).children("div").html('Hide Playlist');
     $j(this).children("span").html('▲');
});

Is there a better way of writting this ? I feel like there's a lot of lines for such a small thing.

Can it be optimized?

Thanks!

share|improve this question

3 Answers 3

up vote 5 down vote accepted

Well, it can be written differently with less repetition, but it's readable as it is. There are 4 things that need to happen when the toggling occurs; your code does exactly that. It's the simplest way to write it, really.

My only real suggestion would be to cache $j(this) in a variable to avoid recreating it.

Regardless, here's a funky rewrite, just for the sake of it

function toggle(hiding) {
  var title   = (hiding ? "Show Playlist" : "Hide Playlist"),
      symbol  = (hiding ? "&#9960"        : "&#9650"),
      method  = (hiding ? "slideUp"       : "slideDown");

  return function () {
    var element = $j(this);
    element.attr("title", title).children("div").html(title);
    element.children("span").html(symbol);
    $j(".playlist-bar-tray")[method]();
  };
}

$j('#hideShowBtn').toggle( toggle(true), toggle(false) );
share|improve this answer
    
I actually really like that solution, thanks a lot! –  Lelly Nov 23 '12 at 13:33
    
@Lelly Glad you found it useful. But do check out ZeroOne's answer too, and read its comments. There's a lot to gain by keeping the content in the markup, and only the behavior in the JS –  Flambino Nov 23 '12 at 17:35

How about "cheating" a bit by creating two copies of those elements whose values should change and then just alternate between which element to show. Something like this:

<div class="toggleable">
    <span>&#9660;</span> Show playlist
</div>
<div class="toggleable" style="display: none;">
    <span>&#9650;</span> Hide playlist
</div>

And then somewhere in your JavaScript you call the .toggle() function from the jQuery UI:

$(".toggleable").toggle();

Now it would look as though the texts changed, while actually you are hiding one element and displaying another.

share|improve this answer
    
I think this is the neater approach. In the original code there is a lot of html/content embedded in the code, so anyone reading the .html would be unable to see what is going on. This approach means the javascript is just the action. –  Quango Nov 23 '12 at 9:03
1  
I agree with @Quango - keep content and behavior separate. Other alternatives would be to use a class and some CSS so show/hide elements (and simply use .toggleClass()), or store the hide/show strings in data-* attributes so they're in the HTML and not the JS. –  Flambino Nov 23 '12 at 9:13

Another option (though I have to say that I like @ZeroOne's solution in this case):

var getToggleHandler = function (opts) {
  return function () {
    var $this = $j(this);
    $j('.playlist-bar-tray')[opts.method]();
    $this.attr("title", opts.title);
    $this.children("div").html(opts.title);
    $this.children("span").html(opts.spanContent);
  };
}

$j('#hideShowBtn').toggle(getToggleHandler({
  method: 'slideUp',
  title: 'Show Playlist',
  spanContent: '&#9660;'
}), getToggleHandler({
  method: 'slideDown',
  title: 'Hide Playlist',
  spanContent: '&#9650;'
}));
share|improve this answer

Your Answer

 
discard

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.