patternjavascriptMinor
Changing element visibility based on multiple checkbox states
Viewed 0 times
statescheckboxvisibilityelementmultiplebasedchanging
Problem
I am writing a script in JavaScript & HTML and have some optimization problems. In the
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
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'd like to rewrite the statements to make it more obvious that the
Second iteration
Let's make it even more obvious by combining everything into one giant conjunction.
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.
I have to say, though, that the
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.