Can please someone review and give me suggestions for improving this code? I am very new to JavaScript.
I need to create a method in JavaScript which will take parameters as
DateRange
(possible values asToday
/This Week
/This Month
BooleanFullDate
(possible values as true/false)
Now, when the parameter value is Today
, it should return
10/23/2013
10/23/2013
When This Week
and BooleanFullDate
are true
, it should return
10/20/2013
10/26/2013
When This Week
and BooleanFullDate
are false
, it should return
10/20/2013
10/23/2013
When This Month
and BooleanFullDate
are true
, it should return
10/01/2013
10/31/2013
When This Month
and BooleanFullDate
are false
, it should return
10/01/2013
10/23/2013
For which I have written my code here.
Lastly, can someone please suggest improvements for the code?
EDIT:- Source code
function GetStartAndEnd(dateRange, bGetFullRange) {
var dateObject = new Date();
var dateobj = new Date()
var month = dateobj.getMonth()+1
var day = dateobj.getDate()
var year = dateobj.getFullYear()
var start = undefined;
var end = undefined;
if (dateRange.toLowerCase() == "today") {
start = month + "/" + day + "/" + year;
end = month + "/" + day + "/" + year;
} else if (dateRange.toLowerCase() == "this week") {
var curr = new Date;
var firstDay = new Date(curr.setDate(curr.getDate() - curr.getDay()));
var lastDay = new Date(curr.setDate(curr.getDate() - curr.getDay() + 6));
var fmonth = firstDay.getMonth()+1;
var fday = firstDay.getDate();
var fyear = firstDay.getFullYear();
start = fmonth + "/" + fday + "/" + fyear;
var lmonth = lastDay.getMonth()+1;
var lday = lastDay.getDate();
var lyear = lastDay.getFullYear();
end = lmonth + "/" + lday + "/" + lyear;
} else if (dateRange.toLowerCase() == "this month" && bGetFullRange) {
var date = new Date(),
y = date.getFullYear(),
m = date.getMonth();
var firstDay = new Date(y, m, 1);
var lastDay = new Date(y, m + 1, 0);
var fmonth = firstDay.getMonth()+1;
var fday = firstDay.getDate();
var fyear = firstDay.getFullYear();
start = fmonth + "/" + fday + "/" + fyear;
var lmonth = lastDay.getMonth()+1;
var lday = lastDay.getDate();
var lyear = lastDay.getFullYear();
end = lmonth + "/" + lday + "/" + lyear;
} else if (dateRange.toLowerCase() == "this month" && !bGetFullRange) {
var date = new Date(),
day = date.getDate(),
y = date.getFullYear(),
m = date.getMonth();
var firstDay = new Date(y, m, 1);
var lastDay = new Date(y, m + 1, 0);
var fmonth = firstDay.getMonth()+1;
var fday = firstDay.getDate();
var fyear = firstDay.getFullYear();
start = fmonth + "/" + fday + "/" + fyear;
end = m+1 + "/" + day + "/" + y;
}
console.log({
"start": start,
"end": end
});
return {
"start": start,
"end": end
}
}
GetStartAndEnd("Today");
GetStartAndEnd("this week");
GetStartAndEnd("this month", true);
GetStartAndEnd("this month", false);
-
1\$\begingroup\$ Please include your code for review. External links can easily go away. \$\endgroup\$ChrisWue– ChrisWue2013年10月23日 07:34:05 +00:00Commented Oct 23, 2013 at 7:34
-
\$\begingroup\$ @ChrisWue Sure! I will do it now. \$\endgroup\$Idothisallday– Idothisallday2013年10月23日 08:41:56 +00:00Commented Oct 23, 2013 at 8:41
-
\$\begingroup\$ OT: But momentjs.com might be of some value ;) \$\endgroup\$Thomas Junk– Thomas Junk2013年10月24日 19:53:20 +00:00Commented Oct 24, 2013 at 19:53
1 Answer 1
I see some issues with your code
- There's a lot of repetition
- Your return values should be actual
Date
objects, and perhaps handle the time component too. Formatting the date is outside the function's responsibilities. Who knows how the formatting or use of the start/end dates might change? But what won't change is that the function should return start and end, so stick to that. - Your arguments are a clue that something's not quite right: The 2nd argument only makes sense if the 1st argument is
"this month"
. Why not respect the 2nd arg for weeks too? - A style thing, but by convention JS functions are
lowerCamelCase
unless they're constructors (i.e. classes). SoDate
is uppercase, as it's a constructor, but your function should begetStartAndEnd
To take a page from the ActiveSupport library for Ruby, which has excellent date/time helpers, it might be more beneficial to break everything up into the separate functions (and encapsulate them in an object to avoid polluting the global namespace).
For instance:
var DateHelper = {
beginningOfDay: function (date) {
var beginning = new Date(date);
beginning.setHours(0, 0, 0, 0);
return beginning;
},
endOfDay: function (date) {
var end = new Date(date);
end.setHours(23, 59, 59, 999);
return end;
},
beginningOfWeek: function (date) {
var beginning = DateHelper.beginningOfDay(date);
beginning.setDate(date.getDate() - date.getDay());
return beginning;
},
endOfWeek: function (date) {
var end = DateHelper.endOfDay(date);
end.setDate(date.getDate() - date.getDay() + 6);
return end;
},
beginningOfMonth: function (date) {
var beginning = DateHelper.beginningOfDay(date);
beginning.setDate(1);
return beginning;
},
endOfMonth: function (date) {
var end = DateHelper.endOfDay(date);
end.setMonth(date.getMonth() + 1, 0);
return end;
}
//... more helper functions?
}
From this, you can build more complex functionality. For instance, your original function could now be written as:
function getStartAndEnd(rangeName, extendToEnd) {
// At minimum, we'll want the range of the current date
var range = {
start: DateHelper.beginningOfDay(new Date),
end: DateHelper.endOfDay(new Date);
};
extendToEnd = extendToEnd || false; // default to false
switch(rangeName.toLowerCase()) {
case "this week":
range.start = DateHelper.beginningOfWeek(range.start);
if( extendToEnd ) {
range.end = DateHelper.endOfWeek(range.end);
}
break;
case "this month":
range.start = DateHelper.beginningOfMonth(range.start);
if( extendToEnd ) {
range.end = DateHelper.endOfMonth(range.end);
}
break;
default:
// do nothing
break;
}
return range;
}
That'll give you start/end date objects that you can format however you want (which, of course, would be a job for another tiny function).
But now that everything's broken up into separate, more focussed functions, you can mix and match to suit your needs. I'd avoid calling functions with string arguments that define what you want back. Probably better to simply have a thisWeek()
function somewhere, or a thisMonth()
function, and leave string interpretation at an even higher level.
Note, by the way, that time and date stuff is always tricky. For instance, all of these functions assume you're talking about the user's local time zone. Depending on your context, that might cause issues. Also, where I live the beginning of a week would be a Monday, not a Sunday. So beware of such things.
-
\$\begingroup\$ Thanks a lott. +1 for explaining so beautifully. Really appreciate it . Please have a look at this fiddle which I have created as per you code, and let me know if its proper. \$\endgroup\$Idothisallday– Idothisallday2013年10月24日 07:20:59 +00:00Commented Oct 24, 2013 at 7:20
-
\$\begingroup\$ Updated Fiddle \$\endgroup\$Idothisallday– Idothisallday2013年10月24日 07:29:59 +00:00Commented Oct 24, 2013 at 7:29
-
\$\begingroup\$ @Idothisallday Well, it certainly looks like it's doing the right thing. Optimally, this is the sort of thing that could be checked with automated tests, but that's a story for another time. But yeah, looks like it's working. Just be sure to remove or otherwise disable the console.log calls in production; no need to spam people's browsers with stuff that's only of interest to you. \$\endgroup\$Flambino– Flambino2013年10月24日 08:42:04 +00:00Commented Oct 24, 2013 at 8:42
-
\$\begingroup\$ @Idothisallday Just fixed my code by the way. Most of the functions were actually ignoring the argument, and using a new Date instead, which wasn't the idea. Sorry about that. \$\endgroup\$Flambino– Flambino2013年10月24日 08:45:30 +00:00Commented Oct 24, 2013 at 8:45