4
\$\begingroup\$

My function calculates days elapsed from the current date. Input is fixed. I will get vector.

Please comment on this.

static bool daysElapsed(const std::vector<char>& vec,int& val)
{       
    std::string str(vec.begin(),vec.end());
    char *pos;// whose value is set by the function to the next character in str after the numerical value.
    int l = strtol(str.c_str(),&pos,10); // Its intention we don’t check till which length strtol converted
    if ((errno == ERANGE && l == LONG_MAX) || l > INT_MAX)      return false;
    if ((errno == ERANGE && l == LONG_MIN) || l < INT_MIN)      return false;

    int year = l/10000;
    int month = (l%10000)/100;
    int date = l%100;

    struct std::tm a = {0,0,0,date,
        month-1,   //Month (0 – 11; January = 0).
        year-1900  //Year (current year minus 1900).
    }; 
    struct std::tm b;               

    time_t rawtime;
    time (&rawtime);                // Get the current calendar time UTC !-> Do not rely on this time, Convert to local time first
    localtime_s (&b,&rawtime);     
    time_t x = std::mktime(&a);
    time_t y = std::mktime(&b);
    if ( x != (std::time_t)(-1) && y != (std::time_t)(-1) ) // Ensure both time are valid
    {
        double difference = std::difftime(y, x) / (60 * 60 * 24);           
        std::cout << "difference = " << difference << " days" << std::endl;
    }
    else
        return false;

    return true;


}
\$\endgroup\$
  • 1
    \$\begingroup\$ One thing I notice is that the vec parameter can be a const& since it's not modified in the function. Alongside that, cbegin() and cend() can instead be used for str. \$\endgroup\$ – Jamal Oct 12 '13 at 7:25
  • \$\begingroup\$ Hi all , I finally dumped this implementation. Rather choose very fast way to calculate.You can convert a date to a Julian day number in O(1). and substracte two. Thanks to this beautiful community . \$\endgroup\$ – Satbir Oct 13 '13 at 11:12
4
\$\begingroup\$

It's weird to take a std::vector<char> instead of a std::string. If you must take a std::vector<char>, then make two overloaded versions of the function, where the vector variant calls the string variant.

You never use the val parameter. You probably meant to use it to return the computed result? However, the computed difference is a double.

strtol() returns a long, but you assign it to an int. Then you check if that int exceeds INT_MAX or is less than INT_MIN — which is impossible by definition. Comparing an int with LONG_MAX and LONG_MIN is similarly nonsensical. Really, all you want to do is check whether errno is non-zero.

You seem to expect the input in YYYYMMDD format, but you don't do any validation other than checking for overflow or underflow. There are plenty of 8-digit numbers that shouldn't be valid dates.

Unfortunately, the localtime_s() function is Windows-only. POSIX has localtime_r(), whose arguments are in the other order, but Windows doesn't support it. You'll probably need an #ifdef _WIN32.

When testing for the validity of x and y, put the early-return failure case first. That's just a better pattern for error handling, because the successful case doesn't have to be indented another level.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your comment , I will revisit implementation \$\endgroup\$ – Satbir Oct 12 '13 at 8:02
  • \$\begingroup\$ I updated post after fixing you comment. thanks a lot. Your comments helped a lot. Also i am only writing for windows. Input format is fixed. \$\endgroup\$ – Satbir Oct 12 '13 at 10:15
  • 2
    \$\begingroup\$ I'm glad you got to incorporate some feedback, but please don't abuse the editing feature by modifying the question after the review — it makes it impossible for other users of this site to follow what happened. I've reverted your revision 6 back to revision 4. If you want more feedback, please post a separate question that mentions this one. \$\endgroup\$ – 200_success Oct 12 '13 at 16:35
  • \$\begingroup\$ By the way, your code is getting really complex after adding validation. The usual way to parse dates and times in C/C++ is to use strftime(). \$\endgroup\$ – 200_success Oct 12 '13 at 16:36
  • \$\begingroup\$ Sure i will take care on editing part... Need to learn wow on this site \$\endgroup\$ – Satbir Oct 13 '13 at 1:39
1
\$\begingroup\$

There is a serious bug in my code:

The function fails if the year is before 1970. mktime handles dates in any timezone from midnight, January 1, 1970, to January 18, 19:14:07, 2038.

See MSDN

\$\endgroup\$
  • \$\begingroup\$ Whether or not you consider that to be a bug depends on what you define as acceptable input. Date/time calculations tend to get really complicated the more situations you want to support. Do you need to handle the Julian-Gregorian transition? Dates before AD 1? You have to draw the line somewhere. \$\endgroup\$ – 200_success Oct 13 '13 at 10:06
  • \$\begingroup\$ My requirement is to count age (in number of days) . So perhaps i should at least support 150 year back from current day to be on safer side :). Anyway if i don't get a suitable API i will write my own:) \$\endgroup\$ – Satbir Oct 13 '13 at 10:31

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.