Changing element visibility based on multiple checkbox states

Question

I am writing a script in JavaScript & HTML and have some optimization problems. In the for, loop I check a variable named visible in every if clause. I think it's meaningless because once I know that the variable value is false there is no need to check the remaining if clauses but skip to the last line. I thought about goto but don't want to use it, and don't know how to fix it.

<script>    
function filterServices(){  
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
    var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;


    for (i = 0; i < serwisy.length; i++) {

        var serwis = serwisy[i];
        var el = document.getElementById(serwis[0]);    
        var visible = true;

        if(download && visible){    visible = serwis[2] == 1 ? true : false;}
        if(audio && visible){   visible = (serwis[3] & (1 << 1)) != 0 ? true : false;}
        if(video && visible){   visible = (serwis[3] & (1 << 2)) != 0 ? true : false;}
        if(offline && visible){ visible = (serwis[3] & (1 << 3)) != 0 ? true : false;}  
        if(free && visible){    visible = (serwis[4] & (1 << 1)) != 0 ? true : false;}
        if(pay && visible){ visible = (serwis[4] & (1 << 2)) != 0 ? true : false;}

        el.style.visibility = visible ? "visible" : "hidden";
        }
    }

Here's my attempt to fix it. However, it was not approved because I am allowed to use only logical operators and no reference to the HTML code.

<script>    
function filterServices(){  
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
        var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;


    for (i = 0; i < serwisy.length; i++) {

        var serwis = serwisy[i];
        var el = document.getElementById(serwis[0]);            

        if(download){   
            if(serwis[2] != 1){ el.style.visibility = "hidden"; continue;}}
        if(audio){  
            if((serwis[3] & (1 << 1)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(video){  
            if((serwis[3] & (1 << 2)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(offline){    
            if((serwis[3] & (1 << 3)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(free){   
            if((serwis[4] & (1 << 1)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(pay){    
            if((serwis[4] & (1 << 2)) != 0){    el.style.visibility = "hidden"; continue;}}

        el.style.visibility = "visible" : "hidden";
        }
    }

Does anyone have any idea how to improve this code?

Well, I've had issues setting el.style.visibility to "visible" in the past. It works to just set it to "". — WebWanderer
– WebWanderer, Commented Dec 8, 2014 at 20:30

200_success · Accepted Answer · 2014-12-08 20:10:14Z

First iteration

You neglected to localize i.

I'd like to rewrite the statements to make it more obvious that the visible flag is something that can be falsified by each if condition.

function filterServices() {
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
    var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;

    for (var i = 0; i < serwisy.length; i++) {
        var serwis = serwisy[i];
        var visible = true;

        if (download) { visible = visible && (serwis[2] == 1); }
        if (audio)    { visible = visible && (serwis[3] & (1 << 1)); }
        if (video)    { visible = visible && (serwis[3] & (1 << 2)); }
        if (offline)  { visible = visible && (serwis[3] & (1 << 3)); }
        if (free)     { visible = visible && (serwis[4] & (1 << 1)); }
        if (pay)      { visible = visible && (serwis[4] & (1 << 2)); }

        document.getElementById(serwis[0]).style.visibility = visible ? "visible" : "hidden";
    }
}

Second iteration

Let's make it even more obvious by combining everything into one giant conjunction.

function filterServices() {  
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
    var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;

    for (var i = 0; i < serwisy.length; i++) {
        var serwis = serwisy[i];
        var visible = (!download || (serwis[2] == 1)))
                   && (!audio    || (serwis[3] & (1 << 1)))
                   && (!video    || (serwis[3] & (1 << 2)))
                   && (!offline  || (serwis[3] & (1 << 3)))
                   && (!free     || (serwis[4] & (1 << 1)))
                   && (!pay      || (serwis[4] & (1 << 2)));

        document.getElementById(serwis[0]).style.visibility = visible ? "visible" : "hidden";
    }
}

Third iteration

But that looks like a mess, especially since the bit operations take a bit of thinking to understand. Let's add some readability back in by introducing functions to explain the bit operations.

function filterServices() {  
    var wantDownload = document.getElementById('download').checked;
    var wantFree     = document.getElementById('free').checked;
    var wantPay      = document.getElementById('pay').checked;
    var wantAudio    = document.getElementById('audio').checked;
    var wantVideo    = document.getElementById('video').checked;
    var wantOffline  = document.getElementById('offline').checked;

    function hasDownload(serwis) { return serwis[2] == 1; }
    function hasAudio(serwis)    { return serwis[3] & (1 << 1); }
    function hasVideo(serwis)    { return serwis[3] & (1 << 2); }
    function hasOffline(serwis)  { return serwis[3] & (1 << 3); }
    function hasFree(serwis)     { return serwis[4] & (1 << 1); }
    function hasPay(serwis)      { return serwis[4] & (1 << 2); }

    for (var i = 0; i < serwisy.length; i++) {
        var serwis = serwisy[i];
        var visible = (!wantDownload || hasDownload(serwis))
                   && (!wantAudio    || hasAudio(serwis))
                   && (!wantVideo    || hasVideo(serwis))
                   && (!wantOffline  || hasOffline(serwis))
                   && (!wantFree     || hasFree(serwis))
                   && (!wantPay      || hasPay(serwis));

        document.getElementById(serwis[0]).style.visibility = visible ? "visible" : "hidden";
    }
}

I have to say, though, that the serwisy data structure doesn't seem well designed. Consider changing it from a two-dimensional array to an array of objects with reasonable names for the keys, so that you can write things like serwisy[i].elementId instead of serwisy[i][0].

Agreed on serwisy, way too old skool, also good catch on var i — konijn
– konijn, Commented Dec 8, 2014 at 20:17
@200_success Thanks for the detailed answer. I think that this might be the solution I need to do or at least it's far better and more clean than my code. — AnhTuan
– AnhTuan, Commented Dec 9, 2014 at 9:47
@200_success Just in case if you are interested - this solution was not accepted as well since it is still far from being "the shortest and most optimal". — AnhTuan
– AnhTuan, Commented Dec 9, 2014 at 13:46

rolfl · Accepted Answer · 2014-12-08 17:54:01Z

Frankly, I don't think the original code is all that bad. The only Issue I really see is the overuse of the ternary operators inside the checks. Where you have, for example:

visible = (serwis[3] & (1 << 1)) != 0 ? true : false;

you should instead have:

visible = (serwis[3] & (1 << 1)) != 0;

For some formatting consistency, I would also swap the order of the boolean checks on each condition:

for (i = 0; i < serwisy.length; i++) {

    var serwis = serwisy[i];
    var el = document.getElementById(serwis[0]);    
    var visible = true;

    if(visible && download){
        visible = serwis[2] == 1;
    }
    if(visible && audio){
        visible = (serwis[3] & (1 << 1)) != 0;
    }
    if(visible && video){
        visible = (serwis[3] & (1 << 2)) != 0;
    }
    if(visible && offline){
        visible = (serwis[3] & (1 << 3)) != 0;
    }  
    if(visible && free){
        visible = (serwis[4] & (1 << 1)) != 0;
    }
    if(visible && pay){
        visible = (serwis[4] & (1 << 2)) != 0;
    }

    el.style.visibility = visible ? "visible" : "hidden";
}

I can't see a way to simplify the logic further, and easily keep it readable. The performance impact of the multiple boolean checks (even if the early ones set visible to false) will be small. There's no real reason to go to extraordinary lengths to prevent it.

Thanks for the comment however this soultion was not accepted and was said to be too complicated. Therefore I have no idea how to simplify it. — AnhTuan
– AnhTuan, Commented Dec 8, 2014 at 19:23

Simon Forsberg · Accepted Answer · 2014-12-08 20:01:03Z

@rolfl can't see a way to simplify it any further, and keeping it readable, but I believe that I can.

What is important to note is that the visible variable will never be set to true after having been set to false. This means that we can rewrite it using boolean AND logic.

var downloadVisible = (serwis[2] == 1) || !download;
var audioVisible = (serwis[3] & (1 << 1)) != 0 || !audio;
var videoVisible = (serwis[3] & (1 << 2)) != 0 || !video;
var offlineVisible = (serwis[3] & (1 << 3)) != 0 || !offline;
var freeVisible = (serwis[4] & (1 << 1)) != 0 || !free;
var payVisible = (serwis[4] & (1 << 2)) != 0 || !pay;

var visible = downloadVisible && audioVisible &&
    videoVisible && offlineVisible && freeVisible && payVisible;
el.style.visibility = visible ? "visible" : "hidden";

If this is also considered too complicated, it might be because of your serwis array and your bitmasks. Not everyone are fans of bitmasks. You might want to consider making this bitmask-stuff a pure Javascript object with some sensible names and properties instead. For example instead of serwis[3] & (1 << 3) you can just have serwisObject.offlineSomething

Additionally, it is not clear what serwisy is at all. I think the language is some Eastern-european language (my best guess would be Polish), but what it means or is used for? No idea. I'd personally recommend using English variable names but if you work with people that does not know much English, I understand that that might be hard for them.

Thanks, this might be the way I need to modify my code. You are also right about the origin of the word serwisy. I forgot to edit it from polish to english before posting but I should probably just write my code in english from the begging. — AnhTuan
– AnhTuan, Commented Dec 9, 2014 at 9:51

konijn · Accepted Answer · 2014-12-08 19:47:24Z

You are pretty close, instead of setting el.style.visibility = "hidden" you could have just set visible = false and then do el.style.visibility = visible ? "visible" : "hidden";

The problem for me is that this code is glossing over the fact that byte flags are used. There are so many magical constants that we could write a teen mystery TV series. And your filter should probably be gettting the serwisy as a parameter. Note: please dont use anything but English for your code, I am guessing it's not up to you.

If I were to write this, I would go for this:

function filterServices(){  

    function isChecked( id ){
      return document.getElementById(id).checked;
    }

    var download = isChecked('download');
    var free = isChecked('free');
    var pay = isChecked('pay');
    var audio = isChecked('audio');
    var video = isChecked('video');
    var offline = isCheckedd('offline');

    //Download flags are in serwis[2]
    var BF_DOWNLOAD = 2,
        HAS_DOWNLOAD = 1 << 1;
    //Audio, video, offline flags are in serwis[3]
    var BF_AVO = 3,
        IS_AUDIO = 1 << 1,
        IS_VIDEO = 1 << 2,
        IS_OFFLINE = 1 << 3;
    //Cost flags are in serwis[4]
    var BF_MONEY = 4,
        IS_FREE  = 1 << 1,
        NOT_FREE = 1 << 2;

    for (i = 0; i < serwisy.length; i++) {

        var serwis = serwisy[i],
            el = document.getElementById(serwis[0]);

        //Assume we will have the hide element
        el.style.visibility = "hidden";      

        if(download && serwis[BF_DOWNLOAD] & HAS_DOWNLOAD ){
          continue;
        }  
        if( audio && serwis[BF_AVO] & IS_AUDIO){
          continue;   
        }
        if( video && serwis[BF_AVO] & IS_VIDEO){
          continue;   
        }
        if( offline && serwis[BF_AVO] & IS_OFFLINE){
          continue;   
        }      
        if( free && serwis[BF_MONEY] & IS_FREE ){
          continue;
        }
        if( pay && serwis[BF_MONEY] & NOT_FREE ){
          continue;
        }      

        el.style.visibility = "visible";
    }

}

The last part you could consider writing like this, but your code reviewer might not like it.

        if( (download && serwis[BF_DOWNLOAD] & HAS_DOWNLOAD) ||
            (audio && serwis[BF_AVO] & IS_AUDIO) ||
            (video && serwis[BF_AVO] & IS_VIDEO) || 
            (offline && serwis[BF_AVO] & IS_OFFLINE) || 
            (free && serwis[BF_MONEY] & IS_FREE ) ||
            (pay && serwis[BF_MONEY] & NOT_FREE ) ){
          continue;
        }

rolfl · Accepted Answer · 2014-12-08 20:22:58Z

Adding a second answer because I have had a somewhat different option to consider. The significant difference is the logical definition of each term to consider, and the fact that the checked-state of the various checkboxes determines the fate of the visibility.

Then, using the previous state of the visible flag in the output is significantly more concise because it removes the if-statements.

function isIgnored(id) {
    !document.getElementById(id).checked;
}

var downloadIgnore = isIgnored('download');
var freeIgnore = isIgnored('free');
var payIgnore = isIgnored('pay');
var audioIgnore = isIgnored('audio');
var videoIgnore = isIgnored('video');
var offlineIgnore = isIgnored('offline');

for (var i = 0; i < serwisy.length; i++) {

    var serwis = serwisy[i];
    var el = document.getElementById(serwis[0]);    
    var visible = true;

    visible = visible && (downloadIgnore || serwis[2] == 1);
    visible = visible && (audioIgnore    || (serwis[3] & (1 << 1)) != 0);
    visible = visible && (videoIgnore    || (serwis[3] & (1 << 2)) != 0);
    visible = visible && (offlineIgnore  || (serwis[3] & (1 << 3)) != 0);
    visible = visible && (freeIgnore     || (serwis[4] & (1 << 1)) != 0);
    visible = visible && (payIgnore      || (serwis[4] & (1 << 2)) != 0);

    el.style.visibility = visible ? "visible" : "hidden";
}

Bergi · Accepted Answer · 2014-12-08 18:01:36Z

0

What you're looking for is the AND operator (with its short-circuiting):

var visible =    (!download || serwis[2] == 1)
              && (!audio    || (serwis[3] & (1 << 1)) != 0)
              && (!video    || (serwis[3] & (1 << 2)) != 0)
              && (!offline  || (serwis[3] & (1 << 3)) != 0)
              && (!free     || (serwis[4] & (1 << 1)) != 0)
              && (!pay      || (serwis[4] & (1 << 2)) != 0)

answered Dec 8, 2014 at 18:01

Bergi

1,7759 silver badges16 bronze badges

Add a comment |

Greg Burghardt · Accepted Answer · 2014-12-08 22:01:06Z

I think starting out with "invisible" and detecting "visible" might clean up the code:

for (var i = 0; i < serwisy.length; i++) {
    var serwis = serwisy[i];
    var el = document.getElementById(serwis[0]);    
    var visible = false;

    if (download && serwis[2] == 1)                   { visible = true; }
    else if (audio    && (serwis[3] & (1 << 1)) != 0) { visible = true; }
    else if (video    && (serwis[3] & (1 << 2)) != 0) { visible = true; }
    else if (offline  && serwis[3] & (1 << 3)) != 0)  { visible = true; }
    else if (free     && (serwis[4] & (1 << 1)) != 0) { visible = true; }
    else if (pay      && (serwis[4] & (1 << 2)) != 0) { visible = true; }

    el.style.visibility = visible ? "visible" : "hidden";
}

By detecting visibility, you can use an if/else if structure to avoid duplicate checks.

Stack Exchange Network

Changing element visibility based on multiple checkbox states

7 Answers 7

First iteration

Second iteration

Third iteration

You must log in to answer this question.

Hot Network Questions

Changing element visibility based on multiple checkbox states

7 Answers 7

First iteration

Second iteration

Third iteration

You must log in to answer this question.

Related

Hot Network Questions