5
\$\begingroup\$

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 as Today/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?

original question posted

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);
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Please include your code for review. External links can easily go away. \$\endgroup\$
    – ChrisWue
    Oct 23, 2013 at 7:34
  • \$\begingroup\$ @ChrisWue Sure! I will do it now. \$\endgroup\$ Oct 23, 2013 at 8:41
  • \$\begingroup\$ OT: But momentjs.com might be of some value ;) \$\endgroup\$ Oct 24, 2013 at 19:53

1 Answer 1

3
\$\begingroup\$

I see some issues with your code

  1. There's a lot of repetition
  2. 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.
  3. 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?
  4. A style thing, but by convention JS functions are lowerCamelCase unless they're constructors (i.e. classes). So Date is uppercase, as it's a constructor, but your function should be getStartAndEnd

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.

\$\endgroup\$
4
  • \$\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\$ Oct 24, 2013 at 7:20
  • \$\begingroup\$ Updated Fiddle \$\endgroup\$ 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
    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
    Oct 24, 2013 at 8:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.