jQuery Double Timepicker

I have one year of JavaScript/jQuery experience but I feel a bit messy in my code. The point is that I make some code that works but I feel that it can be improved like hell. I look for that, but even if it's sometimes cleaner I don't feel that it's good.

This is a double timepicker component. The selects are customized and I push the hours with a for loop. I then create a template in order to push them in the DOM and on click the select value changes. I feel that there are a lot a mistakes here.

Here is an example. I tried to make a reusable timepicker component in jQuery.

Demo



`var DoubleTimepicker = function(ctx) {
//Get context
var _ctx = ctx;

//Create array of hours
function arrayHour() {
var demiHours = ["00", "30"];
var times = [];

for (var i = 0; i " + hour + "";
return templateTime;
});
}
//get ehours and push them into ul li template
var fillSelect = function(hours) {
return hours.map(function(hour, key) {
var templateSelect = "" + hour + "";
return templateSelect;
});
}

//toggle hide/show
$('.drop-time', ctx).hide();
var inputTime = $('.input-time', ctx);
inputTime.click(function() {
$('.drop-time', inputTime).toggle();
});
$('.drop-time', ctx).click(function(e) {
e.stopPropagation();
})

var temp = getHours(arrayHour());
var tempSelect = fillSelect(arrayHour());

//transform for the DOM
var divTime = tempSelect.join('');
var optionsTime = temp.join('');

//var for selects
var timeSelect = $('.select_time', ctx);
//var for dropdown div
var divSelect = $('.fromTo', ctx);

timeSelect.append(optionsTime);
divSelect.append(divTime);

var from = $('.input-time .time-from', ctx);
var to = $('.input-time .time-to', ctx);

var fromSelect = $('.select_time-from', ctx);
var toSelect = $('.select_time-to', ctx);

$('.drop-time .from li', ctx).click(function() {
var dataFrom = $(this).data('hour');
$('option', fromSelect).each(f

Your code looks very clean, so we guess you're concerned about working properly, and this is pretty right.

But, no offense, beyond the apparence your code is a bit messy.

So let's look at what could be improved.

Readability

Use significant names (you did so for some of them, but not all), e.g.:

• arrayHour seems to talk about an hour value regarding some array, while it's an array of hour values: you'd better naming it hoursArray

• avoid mixing English with another language like in demiHours, which should be halfHours (BTW note: I'm French like you, so I'm used to fall in the trap :)

• merely don't declare variables that will not be used, like var _ctx = ctx;

Organization

You have three sub-functions, one merely declared as a function and the two other ones as variables (var getHours = function(), var fillSelect = function()): you should be consistent and adopt one of these two ways for all of them.

BTW note that you inverted the presence/absence of the final ;.

Here it's useless:

function arrayHour() {
    ...
}; // <--

while here it's required (while not mandatory, and it's why your code works though):

var getHours = function() { // same for fillSelect()
    ...
} // <--

In the other hand, let's go back to the (anyway useless) var _ctx = ctx;.

Be careful with that if you plan to furtherly evolve to strict mode ('use strict';), since it'll fire a syntax error (see this page):


strict mode prohibits function statements not at the top level of a script or function.

Reducing code

You can reduce your code (although being more readable at the same time), by avoiding to break it with successive transformations and declaring several variables never used elsewhere.

So this whole sequence:

var temp = getHours(arrayHour());
var tempSelect = fillSelect(arrayHour());

//transform for the DOM
var divTime = tempSelect.join('');
var optionsTime = temp.join('');

//var for selects
var timeSelect = $('.select_time', ctx);
//var for dropdown div
var divSelect = $('.fromTo', ctx);

timeSelect.append(optionsTime);
divSelect.append(divTime);

can be replaced by these two simple statements:

$('.select_time', ctx).append(getHours(arrayHour()).join(''));
$('.fromTo', ctx).append(fillSelect(arrayHour()).join(''));

NOTE: this is not an absolute rule, though. At the opposite, you may sometimes face situations where breaking a long statement will improve readability.

Another example:

var strFrom = "";
$('option', fromSelect).removeAttr('selected');
$(this).attr('selected', 'selected');
strFrom = $(this).text() + " ";
from.text(strFrom);

where strFrom is quite useless (the same applies to the other sequence with strTo):

$('option', fromSelect).removeAttr('selected');
$(this).attr('selected', 'selected');
from.text($(this).text() + " ");

Another realm where you can simplify the code while increasing readability is illustrated by this sequence:

var time = i + ":" + demiHours[j];
if (i < 10) {
  time = "0" + time;
}

which can be advantageously replaced by:

var time = (i + 100).toString().substr(1) + demiHours[j];

Finally here is a last case, where this whole function:

function arrayHour() {
  var demiHours = ["00", "30"];
  var times = [];

  for (var i = 0; i < 24; i++) {
    for (var j = 0; j < 2; j++) {
      var time = i + ":" + demiHours[j];
      if (i < 10) {
        time = "0" + time;
      }
      times.push(time);
    }
  }
  return times;
};

might be faster with this reduced code:

var hoursArray = function() {
  return Array(24).fill(0).reduce(function(result, val, i) {
    var hour = (i + 100).toString().substr(1) + ':';
    return result.concat([hour + '00', hour + '30']);
  }, []);
}

In case you're a bit surprised by Array(24).fill(0): it's a simple way to directly create an array of the required size, which serves as base for reduce() to get what we need. But fill(0) must be executed first, because reduce() doesn't process undefined items.

Efficiency

You should take care of multiple execution of a task when it's useless.

An example here is in the two statements we already examined above:

$('.select_time', ctx).append(getHours(arrayHour()).join(''));
$('.fromTo', ctx).append(fillSelect(arrayHour()).join(''));

This way, we call arrayHour() twice... to get the same result!

So we can avoid consuming time with a intermediate variable:

var hoursList = arrayHour();
$('.select_time', ctx).append(getHours(hoursList ).join(''));
$('.fromTo', ctx).append(fillSelect(hoursList ).join(''));

so the function is executed only once.

But there is more: in this precise case, fortunately, we already turned the function body to a one-statement. So we can even do without function:

```
var hoursList = Array(24).fill(0).reduce(function(result, val, i) {
var hour = (i + 100).toString().substr(1) + ':';
return result.concat([hour + '00', hour + '30']);
}, []
);

StackExchange Code Review Q#147998, answer score: 2