# Having less code for the current program

I am was writing a practice program in javascript and it turned out to work quite alright, however I am not entirely happy with the code. I can't shake the feeling that there is a lot of code which doesn't have to be there, I feel that it can be improved.

The idea of the program is basically, you have a selector, each time you make a choice and hit submit a new set of choices are offered. The choices of the user are stored in an object.

I am interested to know if there is a way to make this code more efficient.

Here is the entire source code (+ the html part)

<!DOCTYPE html>
<html>
<meta charset="utf-8" />
<title>Dynamic Selector</title>
<body>
<select id="select">
</select>
<button onclick="updateObject()">Submit</button>
</body>
</html>
<script>
var select = document.getElementById("select");

var choice = { time: "", category: "", complexity: "" };
var time = ["15", "30", "60", "60+"];
var category = ["knowedge", "experimenting", "self-iprovement", "cooking", "health"];
var complexity = [1, 2, 3, 4];

var coef = 0;

//creates the children for the time choices
function setChoices() {
if (coef === 0) {
for (var i = 0; i < time.length; i++) {
var node = document.createElement('option');
node.value = time[i].toString();
node.innerHTML = time[i];
select.appendChild(node);
}
} else if (coef === 1) {
for (var i = 0; i < category.length; i++) {
var node = document.createElement('option');
node.value = category[i].toString();
node.innerHTML = category[i];
select.appendChild(node);
}
} else if (coef === 2) {
for (var i = 0; i < complexity.length; i++) {
var node = document.createElement('option');
node.value = complexity[i].toString();
node.innerHTML = complexity[i];
select.appendChild(node);
}
}
}

function updateObject() {
if (coef === 0) {
choice.time = select.value;
console.log(choice.time);
coef++;
removeChildren();
setChoices();
} else if (coef === 1) {
choice.category = select.value;
coef++;
removeChildren();
setChoices();
} else if (coef === 2) {
choice.complexity === select.value;
printObject();
setChoices();
}
}

//removes the children of the "select" element
function removeChildren() {
while (select.firstChild) {
select.removeChild(select.firstChild);
}
}

function printObject() {
console.log("foobarr");
}

removeChildren();
setChoices();
</script>


Applying the good ol' DRY principle and by adding coef as a parameter, we'd get something like :

function setChoices(coef) {
var relevantArray = (coef === 0) ? time :
(coef === 1) ? category :
complexity;
for (var i = 0; i < relevantArray.length; i++) {
var node = document.createElement('option');
node.value = relevantArray[i].toString();
node.innerHTML = relevantArray[i];
select.appendChild(node);
}
}

function updateObject() {
for (var coef = 0; coef<3; coef++)
{
((coef === 0) ? choice.time :
(coef === 1) ? choice.category :
choice.complexity) =  select.value;
removeChildren();
setChoices(coef);
}
}


(Also, I have changed a few things which I assumed to be wrong, for instance, updateObject did not call setChoices with coef=0.

• Just to be sure I understand this: You are creating a new array(and using it later on) containing the data of one of the 3 arrays, depending on the value of 'coef' right ? – Bloodcount Jul 15 '13 at 7:35
• That's the idea indeed but it shouldn't create a new array, it should just decide at the beginning the one we are going to use afterwards. – SylvainD Jul 15 '13 at 7:54

The most obvious change I would recommend is to refactor the for loop in setChoices. You're using the same code three times.

I'd write it somehow like this:

// It's better not to rely on globals but to pass them to your functions
// (easier to test)
// It's always a good habit to validate your arguments
if (!arrItems || arrItems.length === 0) {
throw new Error("ArgumentNullException: 'arrItems' is null or has no items.")
}

if (!select) {
throw new Error("ArgumentNullException: 'select' is null.")
}

for (var i = 0; i < arrItems.length; i++) {
var node = document.createElement('option');
node.value = arrItems[i].toString();
node.innerHTML = arrItems[i];
select.appendChild(node);
}
}

//creates the children for the time choices
function setChoices() {
switch (coef) {
case 0:
break;
case 1: