Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am looking for ways to enhance this function in C++.

The function gets a date and returns the total number of seconds since the epoch (01/01/1970).

Do you have any suggestion regarding the algorithm, the code style, etc ?

Note: I use this function instead of the standard library functions because it can handle negative dates and dates before 1900.

static int NDaysInMonth[] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };

double getUnixTimeStamp(int y, int m, int d, int h, int mn, int s)
{
    // Begin bounds checking
    if (y < 0 || m < 1 || d < 1 || h < 0 || mn < 0 || s < 0)
        return -1;
    // End bounds checking

    int ye = 1970; //UNIX epoch 01/01/1970

    int sign = y > ye ? 1 : -1;

    int maxY = std::max(y, ye);
    int minY = std::min(y, ye);

    int nleapYears = 0;

    for (int i = minY; i < maxY; i++)
        if ((i % 100 != 0 && i % 4 == 0) || i % 400 == 0) nleapYears++;

    if (m > 2)
        if ((y % 100 != 0 && y % 4 == 0) || y % 400 == 0)
            nleapYears += sign;

    nleapYears = std::max(0, nleapYears);

    double monthsNDays = 0;

    for (int i = 1; i < m; i++)
        monthsNDays += NDaysInMonth[i - 1];

    return ((y - ye) * 365 + sign * nleapYears + monthsNDays + d - 1) * 86400
        + h * 3600 + mn * 60 + s;
}
share|improve this question
1  
Its a leap year if divisible by 400. Thus 1900 is not but 2000 was a leap year. –  Loki Astari May 9 '12 at 18:07
    
If you are before 1970 then monthsNDays is wrong (as you will be counting backwards from Dec-31) –  Loki Astari May 9 '12 at 18:10
    
You forget to check if this year is a leap year (and only apply it after Feb-29). –  Loki Astari May 9 '12 at 18:11
    
What about leap seconds. There have been 25 since 1970. –  Loki Astari May 9 '12 at 18:11
1  
Have you looked at: mktime() –  Loki Astari May 9 '12 at 18:19

1 Answer 1

up vote 4 down vote accepted

Edit: I very wrongly accused Jacobi of posting a flawed algorithm. So I removed my ranting and replaced by:

I had a try compiling your code and I worked on it to represent how I would have written it. See below. I hope this gives you hints about a different possible coding style. I hope to demonstrate some benefits. The main differences are:

  • Comment for readability. In six months you might be very much in doubt yourself of all the intricacies of an algorithm. I had a hard time deciphering your algorithm. I am also in favor of including references to the definitions of formulas etc, which allows reviewers to check them more quickly.
  • A good practice that I still don't manage to discipline myself to is to write commenting immediately for doxygen or another doc generator, so you can save yourself the hassle later. It usually forces you to document all parameters, return values, ... which is good practice anyway.
  • Code formatting to represent the structure of the code. I am a big fan of lots of spacing and horizontal alignment to spot the difference between similar lines easily.
  • I would refactor to avoid repeating the same code twice, even in the case of isLeapYear. I think the algorithm also becomes more readable when you change the condition by a clear function name. For reading how the algorithm works, I actually don't care about the implementation of the leapyear test.
  • Instead of testing for m < 2 to add or remove extra leapyears, I think it is beneficial to try and represent the reality in your code. I do this here by creating N_DAYS_IN_LEAP_MONTH. This benefits not only in simplifying the code, it also serves in multiple places, as now the input validation is more correct as well.
  • Returning -1 on error is misleading because it could also be a valid outcome. I would propose in any case to return 0 on error instead of -1, so it can be distinguished from valid output with a Boolean test as if( x = getUnixTimeStamp(...) ) -> valid. If you want to garantee no ambiguity, you should probably return a struct with an error status, an error message and a result.
  • Personally I prefer longer variable names for clarity, but I suppose that has to do with taste. I also changed nleapyears to nleapdays, which corresponds more to what is is.
  • I try to use const where I can. In this example it is a bit trivial, and compilers would probably compile it to the exact same assembler code without these consts, so you have to see if you find it worth the extra code. If your parameters aren't basic types, but objects however starting to take them as const refernces really starts making a difference.

Not implemented but advised:

  • Allow supporting inputs for the whole range of double. This makes your function more general, and thus more useful. If a program logic needs to restrict more, that should not be done in this function.

#include <algorithm>
#include <iostream>


double getUnixTimeStamp( int year, int month, int day, int hour, int minute, int second );


int main()
{
    std::cout << std::fixed << getUnixTimeStamp( 1979, 12, 31, 23, 59, 59 ) << std::endl;

    return 0;
}



// start of getUnixTimeStamp code
static const int N_DAYS_IN_MONTH     [] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
static const int N_DAYS_IN_LEAP_MONTH[] = { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };


// Is the passed in year a leap year?
// https://en.wikipedia.org/wiki/Leap_year#Algorithm
inline
bool
isLeapYear( const int year )
{
    return

             year % 400 == 0
        || ( year % 4   == 0  &&  year % 100 != 0 )

    ;
}



// returns the epoch timestamp for a given date in UTC
// reference: https://en.wikipedia.org/wiki/Unix_time

// current minimum supported date = 0000-01-01 00:00:00
// current maximum supported date = 2999-12-31 59:00:00
// returns 0 on faulty input, as well as for epoch input

// TODO: support full range of double rather than limiting arbitrarily


double
getUnixTimeStamp( const int year, const int month, const int day, const int hour, const int minute, const int second )
{
    // Do some bounds checking
    // Note that double will still not be indefinite, and you should check what the boundaries for double are.
    // (this also changes per implementation, so you need to check limits.h or something like that)
    // It would be more logical and correct to limit to the range permitted by double than to arbitrarily limit a 0y.
    // If your program needs this limit, you should enforce it outside this function, because it doesn't belong here.

    const int* const DAYS_PER_MONTH = isLeapYear( year ) ? N_DAYS_IN_LEAP_MONTH : N_DAYS_IN_MONTH;

    if
    (
            year   < 0  ||  year   > 3000
        ||  month  < 1  ||  month  > 12
        ||  day    < 1  ||  day    > DAYS_PER_MONTH[ month-1 ]
        ||  hour   < 0  ||  hour   > 24
        ||  minute < 0  ||  minute > 60
        ||  second < 0  ||  second > 60
    )

        return 0;
        // it is problematic to do error handling with the return value, as all possible return values could be valid epochs as well.
        // at least by taking 0 you can use a boolean check like if( x = getUnixTimeStamp(...) ) -> valid.
        // The check will fail for 1970-01-01-00-00-00



    // Calculate the seconds since or to epoch
    // Formula for dates before epoch will subtract the full years first, and then add the days passed in that year.

    int epoch       = 1970                  ; // UNIX epoch 01/01/1970
    int sign        = year > epoch ? 1 : -1 ; // So we know whether to add or subtract leap days

    int nleapdays   = 0                     ; // The number of leap days
    int monthsNDays = 0                     ; // The number of days passed in the current year


    // Count the leap days
    for( int i = std::min( year, epoch );  i < std::max( year, epoch );  i++ )

        if( isLeapYear( i )  )

            ++nleapdays;


    // Calculate the number of days passed in the current year
    for( int i = 1;  i < month;  i++ )

        monthsNDays += DAYS_PER_MONTH[ i - 1 ];



    return

        (
              ( year - epoch )  * 365       // add or subtract the full years
            +   sign            * nleapdays // add or subtract the leap days
            +   monthsNDays                 // The number of days since the beginning of the year for full months
            +   day - 1                     // The number of full days this month

        )         * 86400                   // number of seconds in 1 day

        + hour    * 3600
        + minute  * 60
        + second
    ;
}
share|improve this answer
    
Hi, have you ever tried the code ? Yes there is flaws, but you definitively haven't understood the code. No you don't need to count backward and yes testing for month > 2 have some sense (when the actual year is leap). –  Ghassen Hamrouni May 11 '12 at 9:11
    
Your comments regarding the style and the structure are correct. But regarding unit testing do you have an example when the function returns incorrect output ? –  Ghassen Hamrouni May 11 '12 at 9:16
    
Hi, I am very sorry. You are completely right. I didn't understand that you were subtracting full years first. I owe you a proper review and have updated my answer. –  nus May 11 '12 at 16:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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