Selectable date filters

By Posted on

Problem

I created a simple app with Backbone and Marionette. In a view I created an onRender function.
I created a page that displays a Report with a date filter, using a select tag and affect to an input when the select tag is chosen.
I made that with Backbone and JQuery. And for date I’m using Moment.js and Pikaday.js.

This is a part of my code

var selectdate = $('#publicdate',this.el)[0];
selectdate.addEventListener("change",function(){
    var value = selectdate.value;
    var date1 = $('#date1',this.el)[0];
    var date2 = $('#date2',this.el)[0];
    if(value==="today"){
        date1.value = moment().add('days').format('DD-MM-YYYY')
        date2.value = moment().add('days').format('DD-MM-YYYY')
        $(date1).attr('disabled',true);
        $(date2).attr('disabled',true);
    } else if(value==="yesterday"){
        date1.value = moment().add(-1,'days').format('DD-MM-YYYY')
        date2.value = moment().add(-1,'days').format('DD-MM-YYYY')
        $(date1).attr('disabled',true);
        $(date2).attr('disabled',true);
    } else if(value==="thismonth"){
        date1.value = moment().add('month').startOf('month').format('DD-MM-YYYY')
        date2.value = moment().add('month').endOf('month').format('DD-MM-YYYY')

        $(date1).removeAttr('disabled',true);
        $(date2).removeAttr('disabled',true);
    } else if(value==="lastmonth"){
        date1.value = moment().add(-1,'month').startOf('month').format('DD-MM-YYYY')
        date2.value = moment().add(-1,'month').endOf('month').format('DD-MM-YYYY')
        $(date1).attr('disabled',true);
        $(date2).attr('disabled',true);
    }
})

Is that code is already clean? Is there another way to make that code shorter? Like using a function or something.

Solution

jQuery

Your code uses jQuery, but you don’t seem to be using any features of jQuery that offer any advantages over the DOM – unless you need to target older browsers you can remove your jQuery dependency and thus make your code better: faster (less abstraction, fewer dependencies to download), more portable, more future-proof, etc).

Simply replace $("#id") with document.getElementById and $(selector) with document.querySelectorAll (you can alias it for brevity if necessary: var q = document.querySelectorAll;).

Time-safety

The moment() function, when called, returns the current time value the moment the function is called – your code keeps on calling the moment() function which could return different values each time it’s called (if there’s a long-enough delay in the interpreter, or if your code executes on a time-unit boundary) – issues like these can cause bugs if midnight at the end of a month happens while your function is executing.

For this reason, you should get the value of “now” only once, at the start of your function, and perform all operations on that value (you should treat it as an immutable value too, even though Date is mutable, as that’s best-practice).

var now = moment();
// ...
date1.value = now.add('days').format('yada');

Culture-sensitivity

You’re using the dd-MM-yyyy pattern – only a few countries use dashes between the components and it is not ISO-8601 compliant. If this is intentional then that’s fine – but if your users are coming from a variety of different countries (with their own date-formatting preferences) then you have two choices:

  1. Use ISO-8601 (yyyy-MM-dd HH:mm:ss) which is unambiguous and as a bonus: is lexicographically sortable.

  2. Use Moment’s built-in localization (“i18n”) support: http://momentjs.com/docs/#/i18n/

To respect your users’ settings, do this:

  1. Get moment-with-locales.js from their website

  2. Modify your initialization code:

    // Get the ISO culture settings for the current user, e.g. "en-US" or "es-MX":
var locale = window.navigator.userLanguage || window.navigator.language;

// Configure moment:
moment.locale( locale );

When using locales, use the “locale formats” rather than specific formats. Moment uses "L" to denote a “numeric date” (e.g. “09/07/2016” in en-GB, “07/09/2016” in en-US, or “2016-07-09” in ISO-8601).

Everything

So your code would become:

var locale = window.navigator.userLanguage || window.navigator.language;
moment.locale(locale);

var selectDate = document.getElementById('publicdate');
selectDate.addEventListener('change', onSelectDateChange);

function onSelectDateChange(e) {
    var date1 = document.getElementById('date1');
    var date2 = document.getElementById('date2');
    var now = moment();
    if( selectDate.value == "today" ) {
        date1.value = now.add('days').format('L');
        date2.value = now.add('days').format('L');
        date1.setAttribute('disabled', 'disabled');
        date2.setAttribute('disabled', 'disabled');
    }
    else if( value == "yesterday" ) {
        // and so on...
    }
}

  1. As jQuery is loaded, why not taking advantage of it. This will make the code short. In the OP code, Vanilla JS is used and that object is wrapped in jQuery which is not necessary. Just use jQuery with selector.
  2. Use an object, that’ll be used to keep the mappings of the values and their corresponding configuration of moment. This will be easy to add new values.
  3. Use on() to bind event on the element
  4. Combine the similar code. Ex. for toady and yesterday and thismonth and lastmonth code is similar. This can be combined with making the difference dynamic.
  5. Use chaining to call multiple methods on the same element, instead of diving into DOM multiple times.
var mappings = {
    today: {
        diff: 0,      --| Difference between today and today
        unit: 'days'  --| is 0 days
    },
    yesterday: {
        diff: -1,     --| Difference between toady and yesterday
        unit: 'days'  --| is -1 days
    },
    thismonth: {
        diff: 0,      --| Difference between today and this month
        unit: 'month' --| is 0 month
    },
    lastmonth: {
        diff: -1,     --| Difference between today and last month
        unit: 'month' --| is -1 month
    }
};

$('#publicdate').on('change', function () {
    // Get value of selected option
    var value = $(this).val();

    var startDate,
        endDate;
    var now = moment();
    var map = mappings[value];

    if (value === 'today' || value === 'yesterday') {
        // Startdate and enddate are same
        startDate = endDate = now.add(map.diff, map.unit).format('DD-MM-YYYY');
    } else if (value === 'thismonth' || value === 'lastmonth') {
        startDate = now.add(map.diff, map.unit).startOf(map.unit).format('DD-MM-YYYY');
        endDate = now.add(map.diff, map.unit).endOf(map.unit).format('DD-MM-YYYY');
    }

    if (startDate && endDate) {
        // Update values and disable the elements
        $('#date1').val(startDate).prop('disabled', true);
        $('#date2').val(endDate).prop('disabled', true);
    }
});

Expand snippet

To make this more small, the mapping can be moved into HTML. The diff and unit can be added on the elements using custom data-* attributes.

Here’s the live Demo: 

$(document).ready(function() {
  $('#publicDate').on('change', function() {
    // Get value of selected option
    var value = $(this).val(),
      $selectedOption = $(this).children(':selected');
    var diff = $selectedOption.data('diff'),
      unit = $selectedOption.data('unit');

    var startDate,
      endDate;
    var now = moment();

    if (value === 'today' || value === 'yesterday') {
      // Startdate and enddate are same
      startDate = endDate = now.add(diff, unit).format('DD-MM-YYYY');
    } else if (value === 'thismonth' || value === 'lastmonth') {
      startDate = now.add(diff, unit).startOf(unit).format('DD-MM-YYYY');
      endDate = now.add(diff, unit).endOf(unit).format('DD-MM-YYYY');
    }

    // Update values and disable the elements
    $('#date1').val(startDate).prop('disabled', true);
    $('#date2').val(endDate).prop('disabled', true);
  });
});
<script src="https://cdn.jsdelivr.net/momentjs/2.14.1/moment.min.js"></script>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<select id="publicDate">
  <option value="today" data-diff="0" data-unit="days">Today</option>
  <option value="yesterday" data-diff="-1" data-unit="days">Yesterday</option>
  <option value="thismonth" data-diff="0" data-unit="month">This Month</option>
  <option value="lastmonth" data-diff="-1" data-unit="month">Last Month</option>
</select>

<div>Start Date:
  <input id="date1" />
</div>
<div>End Date:
  <input id="date2" />
</div>

Expand snippet

Good question,

there is so much repetition in there, violating the DRY principle.

I think the first thing is that you apply a number of things to both date1 and date2, you can capture both of them with something like var bothDates = $('#date1, #date2');

The second thing is the constant repetition of $(date1).attr('disabled',true);. I assume there is a free selection choice in publicdate and try to remove repetition that way.

Furthermore;

  • You should try to stick to 1 type of quotes, I think most folks use single quotes.
  • Always lowerCamelCase: selectdate -> selectDate
  • You provide this.el as a context, if you do this because the date1 id is not unique, then you have to redesign your page if you want to have a clean design. If date1 is unique, then I would not provide that context and always use the jQuery val functionality.

I would go for something like this:

var selectDate = document.getElementById('publicdate'); //Assuming the ID is unique
selectDate .addEventListener("change",function(){
    var value = selectDate .value,
        date1 = $('#date1',this.el),
        date2 = $('#date2',this.el),
        bothDates = $('#date1, #date2');
    bothDates.attr('disabled',true);
    if(value==="today"){
        bothDates.val( moment().add('days').format('DD-MM-YYYY') );
    } else if(value==="yesterday"){
        bothDates.val( moment().add(-1,'days').format('DD-MM-YYYY') );
    } else if(value==="thismonth"){
        date1.value = moment().add('month').startOf('month').format('DD-MM-YYYY')
        date2.value = moment().add('month').endOf('month').format('DD-MM-YYYY')    
    } else if(value==="lastmonth"){
        date1.value = moment().add(-1,'month').startOf('month').format('DD-MM-YYYY')
        date2.value = moment().add(-1,'month').endOf('month').format('DD-MM-YYYY')
    } else {
        bothDates.attr('disabled',false);
    }
})

1. Combining JQuery Selectors

Jquery can select multiple items at once as you can with css selectors. This can cut out some duplication.

2. Using Switches instead of if-else chains

Switches are a nice way to make if-else (on a single var) chains look more appealing. (There are some minor caveats but they’re not worth mentioning here.) As you’re changing the behavior based on the value of ‘value’, you can switch on this variable.

3. Using a variable to prevent repeated calls of ‘moment()’

var selectdate = $('#publicdate',this.el)[0];
selectdate.addEventListener("change",function(){
var value = selectdate.value;
var date1 = $('#date1',this.el)[0];
var date2 = $('#date2',this.el)[0];
var bothDates = $('#date1, #date2');
var now = moment();
bothDates.attr('disabled', true);
switch(value)
{
case "today":
    bothDates.value = now.add('days').format('DD-MM-YYYY');
break;
case "yesterday":
    bothDates.value = now.add(-1,'days').format('DD-MM-YYYY');
break;
case "thismonth":
    date1.value = now.add('month').startOf('month').format('DD-MM-YYYY');
    date2.value = now.add('month').endOf('month').format('DD-MM-YYYY');
    bothDates.removeAttr('disabled', true);
break;
case "lastmonth":
    date1.value = now.add(-1,'month').startOf('month').format('DD-MM-YYYY');
    date2.value = now.add(-1,'month').endOf('month').format('DD-MM-YYYY');
break;
}});

  1. #id selectors work globally and don’t need a parent element in $()
  2. Use jQuery fully and don’t touch the underlying DOM elements and events.
    Note that attr('disabled', false) is the same as removeAttr('disabled')
  3. Avoid repetition of constants like the format string
  4. Don’t reinitialize moment() each time, reuse the value.
$('#publicdate').on('change', function() {
    var now = moment();
    var format = 'DD-MM-YYYY';
    var selector = $(this).val();
    switch (selector) {
        case 'today':
        case 'yesterday':
            $('#date1, #date2')
                .val(now.add(selector == 'today' ? 0 : -1, 'days').format(format))
                .attr('disabled', true);
            break;
        case 'thismonth':
        case 'lastmonth':
            var month = now.add(selector == 'thismonth' ? 0 : -1, 'month');
            $('#date1').val(month.startOf('month').format(format))
            $('#date2').val(month.endOf('month').format(format))
            $('#date1, #date2').attr('disabled', selector == 'lastmonth');
            break;
    }
});

Leave a Reply

Your email address will not be published. Required fields are marked *