Slim down Javascript MomentJS Function

Question

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

}}};

Joe Clay · Accepted Answer · 2016-10-27 11:41:31Z

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/

\$\begingroup\$ @MelissaDoll: No problem :) \$\endgroup\$

Joe Clay
– Joe Clay

2016-10-28 09:36:07 +00:00
Commented Oct 28, 2016 at 9:36 — Joe Clay
– Joe Clay, Commented Oct 28, 2016 at 9:36

Stack Exchange Network

Slim down Javascript MomentJS Function

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Slim down Javascript MomentJS Function

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions