12
\$\begingroup\$

I am trying to implement two simple convertors: date/time to time-stamp and vice-versa, without any dependencies on time library routines (such as localtime, mktime, etc, mainly due to the fact that some of them are not thread-safe).

I have the following date/time structure:

typedef struct
{
    unsigned char second; // 0-59
    unsigned char minute; // 0-59
    unsigned char hour;   // 0-23
    unsigned char day;    // 1-31
    unsigned char month;  // 1-12
    unsigned char year;   // 0-99 (representing 2000-2099)
}
date_time_t;

I would like to have a second opinion on the following conversion routines (given a legal input):

static unsigned short days[4][12] =
{
    {   0,  31,  60,  91, 121, 152, 182, 213, 244, 274, 305, 335},
    { 366, 397, 425, 456, 486, 517, 547, 578, 609, 639, 670, 700},
    { 731, 762, 790, 821, 851, 882, 912, 943, 974,1004,1035,1065},
    {1096,1127,1155,1186,1216,1247,1277,1308,1339,1369,1400,1430},
};


unsigned int date_time_to_epoch(date_time_t* date_time)
{
    unsigned int second = date_time->second;  // 0-59
    unsigned int minute = date_time->minute;  // 0-59
    unsigned int hour   = date_time->hour;    // 0-23
    unsigned int day    = date_time->day-1;   // 0-30
    unsigned int month  = date_time->month-1; // 0-11
    unsigned int year   = date_time->year;    // 0-99
    return (((year/4*(365*4+1)+days[year%4][month]+day)*24+hour)*60+minute)*60+second;
}


void epoch_to_date_time(date_time_t* date_time,unsigned int epoch)
{
    date_time->second = epoch%60; epoch /= 60;
    date_time->minute = epoch%60; epoch /= 60;
    date_time->hour   = epoch%24; epoch /= 24;

    unsigned int years = epoch/(365*4+1)*4; epoch %= 365*4+1;

    unsigned int year;
    for (year=3; year>0; year--)
    {
        if (epoch >= days[year][0])
            break;
    }

    unsigned int month;
    for (month=11; month>0; month--)
    {
        if (epoch >= days[year][month])
            break;
    }

    date_time->year  = years+year;
    date_time->month = month+1;
    date_time->day   = epoch-days[year][month]+1;
}

I have tested this on an extensive amount of legal input (between 01/01/2000 and 31/12/2099). Any constructive comments would be appreciated (performance improvement suggestions, readability, etc).

\$\endgroup\$
  • 1
    \$\begingroup\$ Your code will break for dates after 2100 (which is not a leap year), and you do not account for any timezones or daylight-savings. \$\endgroup\$ – rolfl Dec 29 '13 at 16:03
  • \$\begingroup\$ Date-time math is hard. I suggest you rewrite an existing test suite for your library, e.g. from the DateTime Perl module's test suite. This should help you iron out most bugs. Note that using an unsigned char for a year opens up Y2K-style bugs, and that some minutes don't have 60 seconds. I also second rolfl's remark about the absence of time-zone awareness. \$\endgroup\$ – amon Dec 29 '13 at 16:34
  • \$\begingroup\$ I explicitly stated in the question that legal input is in the range 2000 - 2099 \$\endgroup\$ – barak manos Dec 29 '13 at 17:47
  • \$\begingroup\$ Use localtime_r() etc. Also see stackoverflow.com/q/2278919/1157100 \$\endgroup\$ – 200_success Dec 29 '13 at 18:04
  • 1
    \$\begingroup\$ For this epoch unix timestamp "1442842561" i'm getting incorrect value for the date. Its shows year=45,month=9,date=20, hour=13, minutes=36, seconds=1. But the date should be 21. Can you please resolve the bug. \$\endgroup\$ – Stolen Skull Sep 21 '15 at 14:00
12
\$\begingroup\$
  1. Good that OP is using 4 simplifications: year 2000-2099, no DST, no leap second, no timezone. So OP knows of code limitations concerning these. Various elements of this function break without those givens.

  2. Make static unsigned short days a const.

  3. Use a long for your epoch as in:

    void epoch_to_date_time(date_time_t* date_time,unsigned long epoch)
    

    as unsigned is only guaranteed to range form 0 to at least 65535 which is insufficient here.

  4. For various functions, consider adding const. There are pros and cons to this, but may be beneficial in your case:

    // unsigned int date_time_to_epoch(date_time_t* date_time)
    unsigned int date_time_to_epoch(const date_time_t* date_time)
    
  5. There exist equation based (non-loop) solutions to the year-day that do not need a table like your unsigned short days[4][12]. Code then looks more complicated, but is faster. Please advise if interested.

  6. Most of the magic numbers like 60, 24 are so well-known that constant or macro substitution seems superfluous. But (365*4+1) may benefit with something like "DaysPer4Years".

\$\endgroup\$
  • 1
    \$\begingroup\$ Minor: unsigned char hour; // 0-59 should be unsigned char hour; // 0-23. \$\endgroup\$ – chux Dec 29 '13 at 23:37
  • \$\begingroup\$ Thank you. 2. Applied just before reading your answer. 5. Started with that approach, but code seemed much more complicated and much less readable; performance is not much different. 6. Thought about it when I wrote the code, but decided against it; macros are good when you want to keep the option of changing them in the future, which is not the case here. \$\endgroup\$ – barak manos Dec 30 '13 at 1:24
  • \$\begingroup\$ @barak mano Thanks for the clear feedback. 6. Macros and constants are useful for self-documentation too. 5. Equation is less space (embedded concern) . \$\endgroup\$ – chux Dec 30 '13 at 1:58
  • \$\begingroup\$ thanks. Since I changed 'days' to constant, it now resides in the code-section (EPROM on my system) instead of in the data-section (RAM on my system). An equation will probably take the same amount of memory (96 bytes) in the code-section anyway. \$\endgroup\$ – barak manos Dec 30 '13 at 2:12
  • \$\begingroup\$ @barak manos I think I can well beat 96 bytes of code. The ymd --> epoch has a code snippet of if (m < March) { m += 12; y--; } d = ((m-March) * (7832/4) + (140/4)) >> (8-2);. Maybe later I'll post the entire thing for review and CC you. A bit busy now. \$\endgroup\$ – chux Dec 30 '13 at 2:43
2
\$\begingroup\$
  • This is too crammed for a single line:

    return (((year/4*(365*4+1)+days[year%4][month]+day)*24+hour)*60+minute)*60+second;
    

    You should split this line somehow and put it in a helper function if it could be reused.

  • Just keep these as two lines:

    unsigned int years = epoch/(365*4+1)*4; epoch %= 365*4+1;
    

    You could also add a bit more whitespace for readability and make it const:

    const unsigned int years = epoch / (365 * 4 + 1) * 4;
    epoch %= 365 * 4 + 1;
    

    The first suggestion also applies to the three lines above this one. It can be easy to mistake them as a single statement. Plus, if you ever need to add additional statements to them, then they'll continue to get longer and hurt readability.

\$\endgroup\$

protected by Community Sep 21 '15 at 14:51

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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