HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Selectable date filters

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
selectablefiltersdate

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:

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

-
Use Moment's built-in localization ("i18n") support: http://momentjs.com/docs/#/i18n/

To respect your users' settings, do this:

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



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

Code Snippets

var now = moment();
// ...
date1.value = now.add('days').format('yada');
// 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 );
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...
    }
}

Context

StackExchange Code Review Q#138121, answer score: 2

Revisions (0)

No revisions yet.