2
\$\begingroup\$

I have a drop down menu that will run a function onchange. The function takes times from an array , adds a specific amount of time , then changes the innerHTML of the table.

The code works , but is there a simpler version? I plan on adding more options to the drop down menu.

HTML

<select id="sel1" onchange="myFunction()">

   <option value="VIC">VIC</option>
   <option value="NSW">NSW</option>

</select>

<table id="myTable" border="1">
<tbody>
    <tr>
        <td>A</td>
        <td>2016-08-24 16:00</td>
        <td>C</td>
    </tr>
    <tr>
        <td>A</td>
        <td>2016-08-24 16:00</td>
        <td>C</td>
    </tr>
</tbody>

Javascript

function myFunction(){

var cellData = ["2016-08-24 15:45", "2016-08-24 16:00"];
var myTable = document.getElementById("myTable");

if (document.getElementById("sel1").value == "VIC") {

var vicTime = [];

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

date = moment(cellData[i]).add(1, 'h').format('MM/DD/YYYY hh:mm a');

vicTime.push(date);

myTable.rows[i].cells[1].innerHTML = vicTime[i];

}}

if (document.getElementById("sel1").value == "NSW") {

var nswTime = [];
var j;

for (j=0; j<cellData.length; j++) {

date = moment(cellData[j]).add(5, 'h').format('MM/DD/YYYY hh:mm a');

nswTime.push(date);

myTable.rows[j].cells[1].innerHTML = nswTime[j];

}}};
\$\endgroup\$
1
\$\begingroup\$

When you find yourself repeating code like this (just with slight differences), that's usually a sign that it's time to separate the logic into its own function so it can be reused. You could also maybe clean up your conditionals using a switch statement too - but that's more of a personal taste thing.

Here's my take on it:

function myFunction() {
    var cellData = ["2016-08-24 15:45", "2016-08-24 16:00"];
    var myTable = document.getElementById("myTable");

    // Defined inside the function so the data/table are in scope -
    // alternatively, you could define it outside and pass them as
    // parameters.
    function setCells(hours) {
        var times = [];

        for (var i = 0; i < cellData.length; i++) {
            var date = moment(cellData[i]).add(hours, 'h').format('MM/DD/YYYY hh:mm a');
            times.push(date);

            myTable.rows[i].cells[1].innerHTML = times[i];
        }
    }

    switch (document.getElementById("sel1").value) {
        case "VIC":
            setCells(1);
            break;
        case "NSW":
            setCells(5);
            break;
    }
}

And a demo of this code in action: https://jsfiddle.net/5q46xu77/1/

\$\endgroup\$
  • \$\begingroup\$ @MelissaDoll: No problem :) \$\endgroup\$ – Joe Clay Oct 28 '16 at 9:36

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.