Is there a more elegant solution than an if-statement with no else clause?

Question

In the following code, if Control (the element that trigers Toggle's first OL) is not Visible it should be set Visible and all other Controls (Controls[i]) so be Hidden.

.js

function Toggle(Control){
    var Controls=document.getElementsByTagName("ol",document.getElementById("Quote_App"));
    var Control=Control.getElementsByTagName("ol")[0];
    if(Control.style.visibility!="visible"){
        for(var i=0;i<Controls.length;i++){
            if(Controls[i]!=Control){

                Reveal("hide",20,0.3,Controls[i]);

            }else{

                Reveal("show",20,0.3,Control);

            };
        };
    }else{

        Reveal("hide",20,0.3,Control);

    };
};

Although the function [Toggle] works fine, it is actually setting Controls[i] to Hidden even if it is already.

This is easily rectified by adding an If statement as in the code below, surely there is a more elegant solution, maybe a complex If condition?

.js

function Toggle(Control){
    var Controls=document.getElementsByTagName("ol",document.getElementById("Quote_App"));
    var Control=Control.getElementsByTagName("ol")[0];
    if(Control.style.visibility!="visible"){
        for(var i=0;i<Controls.length;i++){
            if(Controls[i]!=Control){

if(Controls[i].style.visibility=="visible"){

                Reveal("hide",20,0.3,Controls[i]);

};

            }else{

                Reveal("show",20,0.3,Control);

            };
        };
    }else{

        Reveal("hide",20,0.3,Control);

    };
};

Your help is appreciated always.

Answer 1

In the ugly pure javascript code world, your solution is fine. But only because you said "elegant", my answer is use jQuery.

I'll write it probably closer to what it really would be, using behaviour-based code rather than event-based, so this won't EXACTLY match your code.. but, it would look something like:

$('#Quote_app ol').click(function() { 
  if ($(this).is(':visible')) {
    $(this).fadeOut();
  } else {
    $(this).fadeIn();
    $('ol', $(this).parent()).not(this).fadeOut();
  }
});

That attaches a click event to every ol element underneath something with ID=Quote_app, and if it's currently visible, hides it, and otherwise, shows it, and hides all other ol elements.

Answer 2

if(Controls[i]!=Control && Controls[i].style.visibility=="visible") {
    Reveal("hide",20,0.3,Controls[i]);
}

Answer 3

Not sure about what means what in your code. Stratagy is to do default action for all items first, and then do specifica action for selected item. Something like this:

for(var i=0;i<Controls.length;i++){
     if(Controls[i].style.visibility=="visible"){

         Reveal("hide",20,0.3,Controls[i]);

     };
}
Reveal("show",20,0.3,Control);

Answer 4

if( Controls[i] != Control ) {
  if( Controls[i].style.visibility == "visible" ){
    Reveal( "hide", 20, 0.3, Controls[i] );
  };
} else {
  Reveal( "show", 20, 0.3, Control );
};

could be rewritten as:

if ( Controls[i] == Control ) {
  Reveal( "show", 20, 0.3, Control );
} else if ( Controls[i].style.visibility == "visible" ) {
  Reveal( "hide", 20, 0.3, Controls[i] );
}

Answer 5

To follow on from the jQuery suggestion -

jQuery often has a toggle function which beomes even more attractive in this situation as it reduces your code to a couple of lines. There currently isnt a toggleFade function but it can be easily added, to quote Karl Swedberg:

You can write a custom animation like this:

  jQuery.fn.fadeToggle = function(speed, easing, callback) { 
    return this.animate({opacity: 'toggle'}, speed, easing, callback); 
  }; 

Then, you can do this:

  $(".bio-toggler").click(function () { 
    $("#bio-form").fadeToggle();
  })

;

Answer 6

This will work without you having to use getComputedStyle, assuming your Reveal("hide", ...) function sets visibility to hidden.

if(Controls [i] !== Control && Controls[i].style.visibility !== "hidden") {
    Reveal("hide", 20, 0.3, Controls[i]);
}

Answer 7

With a little monkey-patching you could make this a lot cleaner without using any external framework. I have also taken the liberty to reshuffle the logic based on the assumption that the ordering of animations (if any) is unimportant.

if Control is hidden
    loop through Controls as C
        hide if C != Control
        show if C = Control
else
    hide Control

Another way to interpret this algorithm is - as long as Controls contains at least one element (doesn't matter which), the visibility of Control will be toggled. And all (Controls minus Control) will be hidden. So I'm again taking the liberty to assume that there will always be one control in Controls, and that Control will always be toggled.

Here's the monkey-patch++ code for it (also on jsfiddle). This eliminates all ifs and elses from the function.

The Toggle function now looks like this:

function Toggle(Control) {
    var Controls = document.getElementsByTagName("ol" ..
    var Control = Control.getElementsByTagName("ol")[0];

    Control.toggle();
    Controls.filter(function(c) { 
        return c != Control && c.isVisible();
    }).hide();
};

Here is the code-behind. NodeList and Array that apply a property on a list of elements:

​NodeList.prototype.forEach = function(f) {
    for(var i = 0; i <​ this.length; i++) {
        f.apply(null, [this[i]]);
    }
};
Array.prototype.forEach = NodeList.prototype.forEach;

NodeList.prototype.filter = function(f) {
    var results = [];
    for(var i = 0; i < this.length; i++) {
        if(f.apply(null, [this[i]])) {
            results.push(this[i]);
        }
    }
    return results;
};
Array.prototype.filter = NodeList.prototype.filter;  

NodeList.prototype.hide = function() {
    this.forEach(function(e) {
        e.hide();
    });
};
Array.prototype.hide = NodeList.prototype.hide;

NodeList.prototype.show = function() {
    this.forEach(function(e) {
        e.show();
    });
};
Array.prototype.show = NodeList.prototype.show;

These methods apply a property on an individual element:

Element.prototype.isVisible = function() {
    return this.style.visibility == 'visible' || this.style.visibility == '';
};

Element.prototype.show = function() {
    this.style.visibility = 'visible';
};

Element.prototype.hide = function() {
    this.style.visibility = 'hidden';
};

Element.prototype.toggle = function() {
    this.isVisible() ? this.hide() : this.show();
};