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

Small Calendar "Widget"

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

Problem

I have been working on a calendar widget for the past hour or so. I wonder if it could be compacted any more than it already is (ignoring whitespace).



`$(document).ready(function() {
var d = new Date();
var cd = d.getDate();
var cm = d.getMonth() + 1;
var cy = d.getFullYear();
var M = cm;
var Y = cy;

var days = ["Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday"];
var months = ["January", "Febuary", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"];

monthDays = function(m, y) {
var d = new Date(y, m, 0).getDate();
return d;
};
firstDay = function(m, y) {
var d = new Date(y, m - 1, 1).getDay();
d = (d === 0) ? 7 : d
return d;
};
currentDay = function(d, m, y) {
var d = new Date(y, m - 1, d).getDay();
d = (d === 0) ? 7 : d
return d;
};

function calendar(M, Y) {
M = M || cm;
Y = Y || cy;
//calendar dates
var date = cd + '/' + ('0' + M).substr(-2) + '/' + ('000' + Y).substr(-4);
var totalDays = monthDays(M, Y);
var lastMonth = monthDays(M - 1, Y);
var firstday = firstDay(M, Y);
var actualDay = currentDay(cd, M, Y);
//counters
var dateCount = 1;
var dayCount = 0;
//prev padding
for (var i = 1; i ');
$('div.widget div.body table tr:last').append('
' + oldDate + '');
dayCount++;
if (dayCount == 7) {
$('div.widget div.body table tr:last').after('');
dayCount = 0;
};
};
//calendar
for (var i = 1; i ');
$('div.widget div.body table tr:last').append('
' + i + '');
dayCount++;
if (dayCount == 7) {
$('div.widget div.body table tr:last').after('');
dayCount = 0;
};
};
//next padding
if (dayCount != 0) {
var nextMonth = 0;
for (var i = dayCount; i
' + nextMonth + '');
if (i == 7)
$('div.widget div.body table tr:last').after('');
};
};
$('div

Solution

What exactly do you mean with "compacted"? Compactness isn't really something you should go for software development, since usually it doesn't have any advantages (performance or otherwise). You should instead be focused on functionality and readability of the source code.

This leads directly to the first point of critic: The variable names. What are cm vs M and cy vs Y? Use long(er), expressive variable names. Also variables shouldn't start with capital letters in JavaScript unless they are classes.

You can always uses a minifier for production code, if you want to improve load times.

monthDays, firstDay and currentDay definitions are lacking the var keyword putting them into the global namespace.

All three could have better names, e.g. monthDays => daysInMonth, firstDay => weekdayOfFirstDay, currentDay => weekday. (Generally you should use the phrases "weekday" and "day of month" in function and variable names. "Day" and "date" alone are too ambiguous.)

Also the functions firstDay and currentDay are almost identical.

The parameters of the function calendar shadow the variables M and Y.

('0' + M).substr(-2) should be extracted into a function, e.g.

function padWithZeros(value, length) {
  var zeros = Array(length).join('0'); // Repeats '0' length-1 times.
  return (zeros + value).substr(-length);
}


dateCount isn't used.

Don't repeat jQuery selectors. Run $('div.widget div.body table') once and store it in a variable.

Instead of searching for $('div.widget div.body table tr:last') each time, store a reference to the table row when you create it:

// before loop
var calenderTable = $('div.widget div.body table');
var currentRow;

// inside loop
  if (dayCount == 0)
    currentRow = $('').appendTo(calenderTable);
  currentRow.append('' + oldDate + '');


$('div.widget div.body table tr:last').after(''); does absolutely nothing. ...append("") already appended the "complete" element. Keep in mind you are manipulating the DOM here, not generating HTML source code. As a reminder you should use ...append("") instead.

The use of paragraphs in the table cells (also later in the HTML) seems unnecessary.

All three loops are virtually identical and I believe the use of the variable currentDay is unnecessary. The weekday should be calculable from the loop index. (EDIT: added missing unnecessary.)

In the first click handler you define trg but don't use it.

widget is a far too generic class name, and I don't understand what trg stands for.

The use of HTML elements seems a bit random. Why is the day of the month a level 1 header and the other parts paragraphs?

Consider using thead and th elements in the table. That way you can also get rid of the duplicate class name head.

Code Snippets

function padWithZeros(value, length) {
  var zeros = Array(length).join('0'); // Repeats '0' length-1 times.
  return (zeros + value).substr(-length);
}
// before loop
var calenderTable = $('div.widget div.body table');
var currentRow;

// inside loop
  if (dayCount == 0)
    currentRow = $('<tr>').appendTo(calenderTable);
  currentRow.append('<td class="prev"><p>' + oldDate + '</p></td>');

Context

StackExchange Code Review Q#101294, answer score: 3

Revisions (0)

No revisions yet.