HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Changing element visibility based on multiple checkbox states

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
statescheckboxvisibilityelementmultiplebasedchanging

Problem

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.

    
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.

```

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

Solution

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].

Code Snippets

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";
    }
}
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";
    }
}
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";
    }
}

Context

StackExchange Code Review Q#72037, answer score: 6

Revisions (0)

No revisions yet.