8
\$\begingroup\$

Below I have code that calculates epoch seconds. It is built to try to change run time checks into asserts in order to give performance in production. I want to make sure all the edge cases are correct, though.

#define DAYSPERWEEK (7)                                                                      
#define DAYSPERNORMYEAR (365U)                                                               
#define DAYSPERLEAPYEAR (366U)                                                               

#define SECSPERDAY (86400UL) /* == ( 24 * 60 * 60) */                                        
#define SECSPERHOUR (3600UL) /* == ( 60 * 60) */                                             
#define SECSPERMIN (60UL) /* == ( 60) */                                                     
#define USECPERSEC (1000000UL)                                                               
#define USECPERMILLI (1000UL)                                                                

#define LEAPYEAR(year)          (!((year) % 4) && (((year) % 100) || !((year) % 400)))       

inline static uint32_t getSecsSinceEpoch(uint32_t aMonth,                             
                                         uint32_t aDay,                               
                                         uint32_t aYear,                              
                                         uint8_t aHour=0,                             
                                         uint8_t aMinute=0,                           
                                         uint8_t aSecond=0) {                         

    const static int leapYearMonthTable[2][12] = {                                    
            {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31},                         
            {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}                          
    };                                                                                

    assert(aMonth < 13);                                                              
    assert(aDay < 32);                                                                
    assert(aYear > 1969 && aYear < 2100); // between epoch start and 2100             
    assert(aHour < 24);                                                               
    assert(aMinute < 60);                                                             
    assert(aSecond < 60);                                                             

    uint16_t epoch = 1970;                                                            
    uint16_t aYears = aYear-epoch;                                                    
    uint32_t countleap = 0;                                                           
    for(uint16_t i = 0; i < (aYears - 1); i++) {                                      
        if (LEAPYEAR((epoch + i)))                                                    
            countleap++;                                                              
    }                                                                                 

    uint32_t secs = aYears * (SECSPERDAY * 365);                                      
    secs += (countleap * SECSPERDAY);                                                 
    secs += aSecond;                                                                  
    secs += (aHour * SECSPERHOUR);                                                    
    secs += (aMinute * SECSPERMIN);                                                   
    secs += ((aDay - 1) * SECSPERDAY);                                                

    if(aMonth > 1) {                                                                  
        uint8_t aDaysPeraMonth = 0;                                                   

        // Only counts when we're on leap aDay or past it                             
        if(LEAPYEAR((epoch + aYears))) {                                              
            if(aMonth > 2) {                                                          
                aDaysPeraMonth = 1;                                                   
            } else if(aMonth == 2 && aDay >= 29) {                                    
                aDaysPeraMonth = 1;                                                   
            }                                                                         
        }                                                                             

        for(uint8_t i = 0; i < aMonth - 1; i++)                                       
            secs += (leapYearMonthTable[aDaysPeraMonth][i] * SECSPERDAY);             
    }                                                                                 
    return secs;                                                                      
} 
\$\endgroup\$
  • 1
    \$\begingroup\$ You could also add main() to show some testing for this code. \$\endgroup\$ – Jamal Jan 26 '15 at 16:47
  • \$\begingroup\$ assert(aDay > 0);assert(aMonth>0); \$\endgroup\$ – Martin York Jan 26 '15 at 19:36
  • \$\begingroup\$ Is this code part of a larger library? DAYSPERWEEK, USECPERSEC, and USECPERMILLI are irrelevant to this task. \$\endgroup\$ – 200_success Jan 26 '15 at 21:16
7
\$\begingroup\$

Get rid of all your #define

#define DAYSPERWEEK (7)                                                                      
#define DAYSPERNORMYEAR (365U)                                                               
#define DAYSPERLEAPYEAR (366U)                                                               

#define SECSPERDAY (86400UL) /* == ( 24 * 60 * 60) */                                        
#define SECSPERHOUR (3600UL) /* == ( 60 * 60) */                                             
#define SECSPERMIN (60UL) /* == ( 60) */                                                     
#define USECPERSEC (1000000UL)                                                               
#define USECPERMILLI (1000UL)                                                                

#define LEAPYEAR(year)          (!((year) % 4) && (((year) % 100) || !((year) % 400))) 

These can all be replaced with scoped variables of const. You can also make the multiplications inline (which you absolutely should do). Those comments are dangerous. What happens if the 86800UL was the value but the comment was /* == ( 24 * 60 * 60) */ did you notice that the value here was wrong? The compiler can not check that the value matches the comments. So never do that (put the calculation in the code).

unsigned long constexpr SecsPerMin   = 60;
unsigned long constexpr SecsPerHour  = (60 * SecsPerMin);
unsigned long constexpr SecsPerDay   = (24 * SecsPerHour);

All of these are compile time evaluated.

Also the macro that expands. This should be a function:

bool isLeapYear(int year) {return !(year % 4) && ((year % 100) || !(year % 400));}

This is a premature optimization that makes the code hard to reason about:

if(aMonth > 1) { 

The code should work just fine without that statement.

This is over-complicated:

    uint8_t aDaysPeraMonth = 0;                                                   

    // Only counts when we're on leap aDay or past it                             
    if(LEAPYEAR((epoch + aYears))) {                                              
        if(aMonth > 2) {                                                          
            aDaysPeraMonth = 1;                                                   
        } else if(aMonth == 2 && aDay >= 29) {                                    
            aDaysPeraMonth = 1;                                                   
        }                                                                         
    }         

You can just say:

    uint8_t aDaysPeraMonth = isLeapYear(epoch + aYears)
                                   ? 1
                                   : 0;

Remember that you are counting the days of the current month separately with this statement:

secs += ((aDay - 1) * SECSPERDAY);  

So your loop only has to deal with months that you have used all the days in.

Why do you try and use every single unsigned format?

inline static uint32_t getSecsSinceEpoch(uint32_t aMonth,                             
                                         uint32_t aDay,                               
                                         uint32_t aYear,                              
                                         uint8_t aHour=0,                             
                                         uint8_t aMinute=0,                           
                                         uint8_t aSecond=0) {   

Pick one that can be used by all the values: unsigned long should do. Use this type everywhere. Then you don't need to worry about extra code converting the types in nearly every expression.

Couple of missing asserts

assert(aDay > 0);
assert(aMonth>0);

Also the use of assert() is not that great. It only works in debug environments. In production code they don't exist. What do you expect to happen if your invariants are broken in production?

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review this helps. I didn't see any comments on the logic being wrong for edge cases and this was my primary concern but also the clean up of types, extra assertions and removal of macros helps the code also. \$\endgroup\$ – bjackfly Jan 29 '15 at 13:39
10
\$\begingroup\$

In C++, you should use const instead of macros for constants:

const int DAYS_PER_WEEK = 7;                                                                    
const int DAYS_PER_NORM_YEAR = 365;                                                               
const int DAYS_PER_LEAPYEAR = 366;                                                               

const long int SECS_PER_DAY = 24 * 60 * 60;                               
const long int SECS_PER_HOUR = 60 * 60;
const unsigned long int SECS_PER_MIN = 60;
const unsigned long int USEC_PER_SEC = 1000000;
const unsigned long int USEC_PER_MILLI = 1000;

This will allow you to use explicit types instead of adding type literals. I've also added underscores to the names as that's the proper convention for compound words in all-uppercase macro names. If these are to be reused, then they could also be wrapped in a namespace to prevent any name-clashing.

As for LEAPYEAR, use a function instead:

int getLeapYear(int year)
{
    return !((year) % 4) && (((year) % 100) || !((year) % 400))
}

Overall, this code looks extremely C-like. If you must use macros for whatever reason, then just program this in C. But if you're indeed using C++, and you have C++11, you can even utilize some newer features from the <chrono> library.

\$\endgroup\$
  • \$\begingroup\$ The question was more of if any edge cases were missed but thanks for the formatting / clean up suggestions \$\endgroup\$ – bjackfly Jan 26 '15 at 19:28
  • \$\begingroup\$ @bjackfly: You're welcome. Keep in mind that any aspect of the code can be reviewed, not just what is requested. If I don't get to this particular question, then someone else may. \$\endgroup\$ – Jamal Jan 26 '15 at 19:47
  • \$\begingroup\$ cool cool thanks again. I implemented your changes in my own copy now :). \$\endgroup\$ – bjackfly Jan 26 '15 at 20:06

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.