9
\$\begingroup\$

The following code is ran when the user presses a button to generate a log file based on the date selected. The DatePicker has a restrictive selected date range of the last 2 weeks including that same day. If the user leaves the page open for more than one day (very common), they will however be able to generate a log for a day that is too old. I've added this check to ensure that this isn't the case. I'm looking for a review on any part of this code.

[Bindable] private var mGeneratedLog:String;
[Bindable] private var mSelectedDate:Date;
[Bindable] private var mSelectableRange:Object;

private function getUploadMessages():void
{
    //validate range
    var now:Date = new Date();
    var tomorrow:Date = new Date(now.fullYear, now.month, now.date + 1, 0, 0, 0, 0);
    var twoWeeksAgo:Date = new Date(now.fullYear, now.month, now.date - 14, 0, 0, 0, 0); 

    //refresh selectable range incase the page has been open too long.
    mSelectableRange = {rangeStart: twoWeeksAgo, rangeEnd: now};

    if (twoWeeksAgo.getTime() >= mSelectedDate.getTime())
    {
        mGeneratedLog = "Selected date is older than 2 weeks of age (Data has been purged).\nLog Not Generated."; 
    }
    else if( mSelectedDate.getTime() >= tomorrow.getTime())
    {
        mGeneratedLog = "Selected date has not happened yet (log would be empty).\nLog Not Generated."; 
    }
    else
    {
        mGeneratedLog = "Generating...";

        var formatter:DateFormatter = new DateFormatter();
        formatter.formatString = "YYYY-MM-DD JJ:NN:SS.QQQ";

        var lParameters:Object = {date:formatter.format(mSelectedDate)}
        mDatalayer.callStatement("GenerateLog", lParameters);
    }
}
\$\endgroup\$
6
\$\begingroup\$
  1. I would store the result of mSelectedData.getTime() into a variable. It will save one method call and remove duplicate code.

  2. The line that resets the data range seems a tad out of place; however, I am not sure about the rest of your implementation, so this might be the best place for it.

  3. Also, I would check your first if-statement:

    if (twoWeeksAgo.getTime() >= mSelectedDate.getTime())
    

    Currently it will evaluate true (which is not expected) if the Dates are the same. From your description, it sounds like you want the date range to be inclusive not exclusive. Change the >= to simply > to get the result you expect:

    if (twoWeeksAgo.getTime() > mSelectedDate.getTime())
    

Other than that, the code looks fine; though I personally would capitalize the first word of each comment and put a space between it and the double slashes. So, // Some comment instead of //some comment. Just a personal preference.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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