jQuery Double Timepicker

Question

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

  //get hours and push them into options select template
  var getHours = function(hours) {
      return hours.map(function(hour, key) {
        var templateTime = "<option value='hour-" + key + "'>" + hour + "</option>";
        return templateTime;
      });
    }
    //get ehours and push them into ul li template
  var fillSelect = function(hours) {
    return hours.map(function(hour, key) {
      var templateSelect = "<li class='time' data-hour='hour-" + key + "'>" + hour + "</li>";
      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(function(index, element) {
      var v = this.value;
      if (v == dataFrom) {
        var strFrom = "";
        $('option', fromSelect).removeAttr('selected');
        $(this).attr('selected', 'selected');
        strFrom = $(this).text() + " ";
        from.text(strFrom);
      }
    })
  })

  $('.drop-time .to li', ctx).click(function() {
    var dataTo = $(this).data('hour');
    $('option', toSelect).each(function(index, element) {
      var v = this.value;
      if (v == dataTo) {
        var strTo = "";
        $('option', toSelect).removeAttr('selected');
        $(this).attr('selected', 'selected');
        strTo = $(this).text() + " ";
        to.text(strTo);
      }
    })
  })
};

var timepick = $('.timepicker_component');
timepick.each(function(index, element) {
  new DoubleTimepicker($(timepick).eq(index));
});

.timepicker_component p {
  margin: 0;
}
.timepicker_component .input-time {
  padding: 10px 15px;
  position: relative;
  border: 2px solid #DCDCDC;
  cursor: pointer;
}
.timepicker_component .time-container {
  position: relative;
}
.fromTo{
  float: left;
  width: 50%;
  }
.timepicker_component .time-container .drop-time {
  background: #fff;
  width: 100%;
  overflow-y: auto;
  overflow-x: hidden;
  height: 150px;
  padding-top: 10px;
  border-top: 2px dotted #dcdcdc;
  margin-top: 6px;
}
.timepicker_component .time-container .drop-time .fromTo li {
  list-style: none;
  cursor: pointer;
  font-size: 14px;
}
.timepicker_component .time-container .drop-time .fromTo li:hover {
  background: #DCDCDC;
}
.timepicker_component .time-container .drop-time .fromTo:first-child {
  border-right: 2px solid #DCDCDC;
}
.timepicker_component select {
  display: none;
}
.timepicker_component .bs-caret {
  position: absolute;
  top: 0;
  right: 2px;
  width: 28px;
  height: 28px;
  margin-top: 4px;
  display: block;
  padding: 8px 0px 0px 2px;
  background: -webkit-linear-gradient(left, #00519E, #009EE0);
  background: linear-gradient(to right, #00519E, #009EE0);
  border-radius: 5px;
  border: none;
}
.timepicker_component .bs-caret span {
  transform: rotate(-90deg);
}

<link href="https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/3.3.7/css/bootstrap.min.css" rel="stylesheet"/>

<div class="timepicker_component col-md-4" style="height:400px">
  <div class="col-md-12">
    <div class="time-container">
      <div class="input-time">
        <p><span class="time-from">00:00</span> à <span class="time-to">00:00</span>
        </p>
        <span class="btn bs-caret"><span class="frh frh-chevron-gauche"></span></span>
        <div class="drop-time">
          <div class="row">
            <div class="fromTo from col-md-6"></div>
            <div class="fromTo to col-md-6"></div>
          </div>
        </div>
        <select class="select_time select_time-from"></select>
        <select class="select_time select_time-to"></select>
      </div>
    </div>
  </div>
</div>

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

Answer 1

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']);
  }, []
);
$('.select_time', ctx).append(getHours(hoursList).join(''));
$('.fromTo', ctx).append(fillSelect(hoursList.join(''));

Others

To be honest I didn't enquired your code further, in its really strategic aspects.

But something puzzles me: apart from the <div class="drop-time"> block, why did you also create the <select> one?
Its sounds like you previously considered this form of layout and actually prefered the current one... but kept building it albeit useless.

Last point, there is yet something weird: why your "from" and "to" <div>s have <li> children?
Even if it's working (thanks to the browsers tolerance), it's clearly an error.