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

jQuery calendar plugin

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

Problem

```
(function( $ ){
var monthNames = [ "Январь", "Февраль", "Март", "Апрель", "Май", "Июнь",
"Июль", "Август", "Сентябрь", "Октябрь", "Ноябрь", "Декабрь" ];

var dayNames = ["Пн","Вт","Ср","Чт","Пт","Сб","Вс"];
var now = new Date();

var methods = {
init : function( options ) {
var settings = $.extend( {
'year' : now.getFullYear(),
'month' : now.getMonth(),
'link' : '%y/%m/%d',
'url' : '',
'dates' : []
}, options);

return this.each(function(){
if (!$(this).data('year')){
$(this).data('year',settings.year);
}
if (!$(this).data('month')){
$(this).data('month',settings.month);
}
if (!$(this).data('link')){
$(this).data('link',settings.link);
}
$(this).data('dates',settings.dates);
$(this).data('url',settings.url);
methods.render($(this));
});

},
destroy : function( ) {

return this.each(function(){
$(this).html('');
})

},
render : function( elem ) {
year = elem.data('year');
month = elem.data('month');
link = elem.data('link');
dates = elem.data('dates');
date = new Date(year,month,1);

elem.html('');
var calendar=elem.find('.calendar');
calendar.append(''+monthNames[month]+' '+year+'');
calendar.append(''+dayNames.join('')+'');
var str = '';
var w

Solution

From a once over:

  • THANK YOU for having all your code and variables in English, very nice



-
In init I would suggest you extract data, fill it in if needed and put it back, so going from this

return this.each(function(){
            if (!$(this).data('year')){
                $(this).data('year',settings.year);
            }
            if (!$(this).data('month')){
                $(this).data('month',settings.month);
            }
            if (!$(this).data('link')){
                $(this).data('link',settings.link);
            }
            $(this).data('dates',settings.dates);
            $(this).data('url',settings.url);
            methods.render($(this));
        });


to this:

return this.each(function(){
            var data = $(this).data() || {};
            data.year = data.year || settings.year;
            data.month = data.month || settings.month;
            data.link = data.link || settings.link;
            data.dates = settings.dates;
            data.url = settings.url;
            $(this).data(data);
            methods.render($(this));
        });


It looks DRY'er and the reduced amount of calls to $(this).data() should make it more efficient as well.

  • You could do the same with year and month in next_month and prev_month.



  • lowerCamelCasing is how JavaScript ought to be done: next_month -> nextMonth,


prev_month -> previousMonth etc.

  • You should really look into a templating routine/library for building your HTML, extracting your HTML templates into vars and making it more readable by splitting over multiple lines. You could even use templating to construct the JSON URL's.



  • From JsHint ( jshint.com )



  • year , month, link, dates etc. are declared in your code without var, polluting the global namespace



  • If you want to compare with 0, you should use === not ==

Code Snippets

return this.each(function(){
            if (!$(this).data('year')){
                $(this).data('year',settings.year);
            }
            if (!$(this).data('month')){
                $(this).data('month',settings.month);
            }
            if (!$(this).data('link')){
                $(this).data('link',settings.link);
            }
            $(this).data('dates',settings.dates);
            $(this).data('url',settings.url);
            methods.render($(this));
        });
return this.each(function(){
            var data = $(this).data() || {};
            data.year = data.year || settings.year;
            data.month = data.month || settings.month;
            data.link = data.link || settings.link;
            data.dates = settings.dates;
            data.url = settings.url;
            $(this).data(data);
            methods.render($(this));
        });

Context

StackExchange Code Review Q#18300, answer score: 3

Revisions (0)

No revisions yet.