2
\$\begingroup\$

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>
<head>
    <meta charset="utf-8" />
    <title>Dynamic Selector</title>
</head>
<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>
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 ? \$\endgroup\$ – Bloodcount Jul 15 '13 at 7:35
  • \$\begingroup\$ 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. \$\endgroup\$ – SylvainD Jul 15 '13 at 7:54
2
\$\begingroup\$

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

Keep your code DRY.

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)
function updateSelectWithItems(select, arrItems) {
    // 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:
            updateSelectWithItems(mySelect, time);
            break;
        case 1:
            updateSelectWithItems(mySelect, category);
            break;
        case 2:
            updateSelectWithItems(mySelect, complexity);
            break;
        default:
            // shouldn't happen
            throw new Error("'coef' has a wrong value.");
            break;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.